| Message ID | YrMsU9HvdBm5YrRH@kili |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: aspeed_udc: fix handling of tx_len == 0 | expand |
> The bug is that we should still enter this loop if "tx_len" is zero. > > After adding the "last" variable, then the "chunk >= 0" condition is no longer > required but I left it for readability. > Use either "chunk >=0" or "last". I think the former is more simpler. > Reported-by: Neal Liu <neal_liu@aspeedtech.com> > Fixes: c09b1f372e74 ("usb: gadget: aspeed_udc: cleanup loop in > ast_dma_descriptor_setup()") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/usb/gadget/udc/aspeed_udc.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/gadget/udc/aspeed_udc.c > b/drivers/usb/gadget/udc/aspeed_udc.c > index d75a4e070bf7..01968e2167f9 100644 > --- a/drivers/usb/gadget/udc/aspeed_udc.c > +++ b/drivers/usb/gadget/udc/aspeed_udc.c > @@ -476,6 +476,7 @@ static int ast_dma_descriptor_setup(struct ast_udc_ep > *ep, u32 dma_buf, { > struct ast_udc_dev *udc = ep->udc; > struct device *dev = &udc->pdev->dev; > + bool last = false; > int chunk, count; > u32 offset; > > @@ -493,14 +494,16 @@ static int ast_dma_descriptor_setup(struct > ast_udc_ep *ep, u32 dma_buf, > "tx_len", tx_len); > > /* Create Descriptor Lists */ > - while (chunk > 0 && count < AST_UDC_DESCS_COUNT) { > + while (chunk >= 0 && !last && count < AST_UDC_DESCS_COUNT) { > > ep->descs[ep->descs_wptr].des_0 = dma_buf + offset; > > - if (chunk > ep->chunk_max) > + if (chunk > ep->chunk_max) { > ep->descs[ep->descs_wptr].des_1 = ep->chunk_max; > - else > + } else { > ep->descs[ep->descs_wptr].des_1 = chunk; > + last = true; > + } > > chunk -= ep->chunk_max; > > -- > 2.35.1
On Thu, Jun 23, 2022 at 01:41:49AM +0000, Neal Liu wrote: > > The bug is that we should still enter this loop if "tx_len" is zero. > > > > After adding the "last" variable, then the "chunk >= 0" condition is no longer > > required but I left it for readability. > > > > Use either "chunk >=0" or "last". > I think the former is more simpler. chunk >= 0 doesn't work. last works but I think this way is more readable. regards, dan carpenter
On Thu, Jun 23, 2022 at 09:43:20AM +0300, Dan Carpenter wrote: > On Thu, Jun 23, 2022 at 01:41:49AM +0000, Neal Liu wrote: > > > The bug is that we should still enter this loop if "tx_len" is zero. > > > > > > After adding the "last" variable, then the "chunk >= 0" condition is no longer > > > required but I left it for readability. > > > > > > > Use either "chunk >=0" or "last". > > I think the former is more simpler. > > chunk >= 0 doesn't work. last works but I think this way is more > readable. Fine, I can remove the chunk >= 0. But you can see why your idea of removing the "last" doesn't work, right? I mean maybe it does work and there was a bug in the original code? Could you please look at that so we're for sure writing correct code? regards, dan carpenter
> On Thu, Jun 23, 2022 at 09:43:20AM +0300, Dan Carpenter wrote: > > On Thu, Jun 23, 2022 at 01:41:49AM +0000, Neal Liu wrote: > > > > The bug is that we should still enter this loop if "tx_len" is zero. > > > > > > > > After adding the "last" variable, then the "chunk >= 0" condition > > > > is no longer required but I left it for readability. > > > > > > > > > > Use either "chunk >=0" or "last". > > > I think the former is more simpler. > > > > chunk >= 0 doesn't work. last works but I think this way is more > > readable. > > Fine, I can remove the chunk >= 0. But you can see why your idea of > removing the "last" doesn't work, right? I mean maybe it does work and > there was a bug in the original code? Could you please look at that so we're > for sure writing correct code? > Why removing the "last" doesn't work? If "chunk == 0", it would go through while loop once, and chunk will be negative (chunk -= ep->chunk_max). Best Regards, -Neal
On Thu, Jun 23, 2022 at 07:52:28AM +0000, Neal Liu wrote: > > On Thu, Jun 23, 2022 at 09:43:20AM +0300, Dan Carpenter wrote: > > > On Thu, Jun 23, 2022 at 01:41:49AM +0000, Neal Liu wrote: > > > > > The bug is that we should still enter this loop if "tx_len" is zero. > > > > > > > > > > After adding the "last" variable, then the "chunk >= 0" condition > > > > > is no longer required but I left it for readability. > > > > > > > > > > > > > Use either "chunk >=0" or "last". > > > > I think the former is more simpler. > > > > > > chunk >= 0 doesn't work. last works but I think this way is more > > > readable. > > > > Fine, I can remove the chunk >= 0. But you can see why your idea of > > removing the "last" doesn't work, right? I mean maybe it does work and > > there was a bug in the original code? Could you please look at that so we're > > for sure writing correct code? > > > > Why removing the "last" doesn't work? If "chunk == 0", it would go through while loop once, and chunk will be negative (chunk -= ep->chunk_max). > chunk -= ep->chunk_max could set chunk to zero. regards, dan carpenter
> On Thu, Jun 23, 2022 at 07:52:28AM +0000, Neal Liu wrote: > > > On Thu, Jun 23, 2022 at 09:43:20AM +0300, Dan Carpenter wrote: > > > > On Thu, Jun 23, 2022 at 01:41:49AM +0000, Neal Liu wrote: > > > > > > The bug is that we should still enter this loop if "tx_len" is zero. > > > > > > > > > > > > After adding the "last" variable, then the "chunk >= 0" > > > > > > condition is no longer required but I left it for readability. > > > > > > > > > > > > > > > > Use either "chunk >=0" or "last". > > > > > I think the former is more simpler. > > > > > > > > chunk >= 0 doesn't work. last works but I think this way is more > > > > readable. > > > > > > Fine, I can remove the chunk >= 0. But you can see why your idea of > > > removing the "last" doesn't work, right? I mean maybe it does work > > > and there was a bug in the original code? Could you please look at > > > that so we're for sure writing correct code? > > > > > > > Why removing the "last" doesn't work? If "chunk == 0", it would go through > while loop once, and chunk will be negative (chunk -= ep->chunk_max). > > > > chunk -= ep->chunk_max could set chunk to zero. > Looks reasonable. Feel free to add: Reviewed-by: Neal Liu <neal_liu@aspeedtech.com> Thanks
On Fri, Jun 24, 2022 at 06:17:31AM +0000, Herrenschmidt, Benjamin wrote: > On Thu, 2022-06-23 at 09:43 +0300, Dan Carpenter wrote: > > On Thu, Jun 23, 2022 at 01:41:49AM +0000, Neal Liu wrote: > > > > The bug is that we should still enter this loop if "tx_len" is > > > > zero. > > > > > > > > After adding the "last" variable, then the "chunk >= 0" condition > > > > is no longer > > > > required but I left it for readability. > > > > > > > > > > Use either "chunk >=0" or "last". > > > I think the former is more simpler. > > > > chunk >= 0 doesn't work. last works but I think this way is more > > readable. > > Hrm... what is that driver ? I've missed it ... is the code lifted from > aspeed-vhub ? If yes, should we instead make it a common code base ? > And if there are bug fixes on one they might apply to the other as > well... No, I'm the person who introduced the bug so it's unique to this driver. regards, dan carpenter
diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c index d75a4e070bf7..01968e2167f9 100644 --- a/drivers/usb/gadget/udc/aspeed_udc.c +++ b/drivers/usb/gadget/udc/aspeed_udc.c @@ -476,6 +476,7 @@ static int ast_dma_descriptor_setup(struct ast_udc_ep *ep, u32 dma_buf, { struct ast_udc_dev *udc = ep->udc; struct device *dev = &udc->pdev->dev; + bool last = false; int chunk, count; u32 offset; @@ -493,14 +494,16 @@ static int ast_dma_descriptor_setup(struct ast_udc_ep *ep, u32 dma_buf, "tx_len", tx_len); /* Create Descriptor Lists */ - while (chunk > 0 && count < AST_UDC_DESCS_COUNT) { + while (chunk >= 0 && !last && count < AST_UDC_DESCS_COUNT) { ep->descs[ep->descs_wptr].des_0 = dma_buf + offset; - if (chunk > ep->chunk_max) + if (chunk > ep->chunk_max) { ep->descs[ep->descs_wptr].des_1 = ep->chunk_max; - else + } else { ep->descs[ep->descs_wptr].des_1 = chunk; + last = true; + } chunk -= ep->chunk_max;
The bug is that we should still enter this loop if "tx_len" is zero. After adding the "last" variable, then the "chunk >= 0" condition is no longer required but I left it for readability. Reported-by: Neal Liu <neal_liu@aspeedtech.com> Fixes: c09b1f372e74 ("usb: gadget: aspeed_udc: cleanup loop in ast_dma_descriptor_setup()") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/usb/gadget/udc/aspeed_udc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)