| Message ID | YrMsU9HvdBm5YrRH@kili |
|---|---|
| State | Not Applicable, archived |
| 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 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... Neal, is that "UDC" IP block the same that resides under the vhub ? If yes then this really needs to be a common driver instead, using the code existing in aspeed-vhub, simply making it able to work without a parent vhub pointer. Cheers, Ben.
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
(switching back to my normal "community" email) On Fri, 2022-06-24 at 09:34 +0300, Dan Carpenter wrote: > > 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. Ok thanks. That said, the code looks fairly similar to the vhub code, so my comment stands, if this is the same IP block, which it appears to be, the driver should be common. IE. The vhub is made of a virtual hub with a bunch of UDCs underneath, this appears to address the ast2600 "new" standalone (no hub) variant of said UDC if I'm not mistaken. The way I structured the code in vhub, it shouldn't be overly difficult to make it standalone. I wrote (and maintain) aspeed-vhub and would be happy to work on this if I got sent an ast2600 board. Cheers, Ben.
> (switching back to my normal "community" email) > > On Fri, 2022-06-24 at 09:34 +0300, Dan Carpenter wrote: > > > 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. > > Ok thanks. That said, the code looks fairly similar to the vhub code, so my > comment stands, if this is the same IP block, which it appears to be, the driver > should be common. > > IE. The vhub is made of a virtual hub with a bunch of UDCs underneath, this > appears to address the ast2600 "new" standalone (no hub) variant of said UDC > if I'm not mistaken. > > The way I structured the code in vhub, it shouldn't be overly difficult to make it > standalone. I wrote (and maintain) aspeed-vhub and would be happy to work > on this if I got sent an ast2600 board. > Hi Ben, This UDC is the independent IP. The ast2600 board can run aspeed-vhub & aspeed_udc simultaneously with different USB port. I think it's no need to restruct the code in vhub.
On Fri, 2022-06-24 at 07:46 +0000, Neal Liu wrote: > > > Hi Ben, This UDC is the independent IP. The ast2600 board can run > aspeed-vhub & aspeed_udc simultaneously with different USB port. > I think it's no need to restruct the code in vhub. But is it a copy of the same base IP block ? IE, is the fundamental HW interface of the independent UDC operating the same way with the same register layout as one of the ports of the vHUB ? I don't like having multiple drivers for the same hardware... if it's different enough, then let's keep it separate, but if not, we should definitely split the udc from the existing vhub code so that the same driver can operate standalone or beneath a vhub. Cheers, Ben.
> On Fri, 2022-06-24 at 07:46 +0000, Neal Liu wrote: > > > > > Hi Ben, This UDC is the independent IP. The ast2600 board can run > > aspeed-vhub & aspeed_udc simultaneously with different USB port. > > I think it's no need to restruct the code in vhub. > > But is it a copy of the same base IP block ? IE, is the fundamental HW interface > of the independent UDC operating the same way with the same register layout > as one of the ports of the vHUB ? > > I don't like having multiple drivers for the same hardware... if it's different > enough, then let's keep it separate, but if not, we should definitely split the udc > from the existing vhub code so that the same driver can operate standalone or > beneath a vhub. > Based on ast2600 hardware design, it's similar, but not exactly the same. I need more time to extract the differences and evaluate it if it could utilize vhub. > Cheers, > Ben.
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(-)