Re: [PATCH] usb: gadget: UMS: fix 64-bit division on ARM32

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Caleb,

On jeu., mars 21, 2024 at 17:01, Caleb Connolly <caleb.connolly@linaro.org> wrote:

> On 21/03/2024 16:48, Mattijs Korpershoek wrote:
>> Hi Caleb,
>> 
>> Thank you for the patch.
>> 
>> On jeu., mars 21, 2024 at 15:28, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>> 
>>> The patch introducing support for dynamic sector sizes changed the types
>>> used in some divisions, resulting in the compiler attempting to use
>>> libgcc helpers (__aeabi_ldivmod). Replace these divisions with calls to
>>> lldiv() to handle this correctly.
>>>
>>> Fixes: 74e56e0c5065 ("usb: gadget: UMS: support multiple sector sizes")
>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> 
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> 
>> Would it be okay if I squashed this into
>> commit 74e56e0c5065 ("usb: gadget: UMS: support multiple sector sizes")
>> ?
>> 
>> I'd like to not (intentionally) send any patches that don't compile to Tom.
>
> For sure, feel free to do that.

Done, here:
https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/304fa0aa445384e5e681a54abf413850591cec10

>> 
>> 
>>> ---
>>> Hi Mattijs,
>>>
>>> Sorry for not catching that sooner! I compile tested this locally and a
>> 
>> No worries, thanks a lot for the quick fix.
>> 
>>> CI pipeline is running here: https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/pipelines/20029
>> 
>> I can see from the above pipeline that some other failures are ongoing,
>> but they are not related to lldiv().
>> 
>> arm-linux-gnueabi-ld.bfd: common/dfu.o: in function `run_usb_dnl_gadget':
>> common/dfu.c:82:(.text.run_usb_dnl_gadget+0xd8): undefined reference to `dm_usb_gadget_handle_interrupts'
>> arm-linux-gnueabi-ld.bfd: common/dfu.c:108:(.text.run_usb_dnl_gadget+0x11c): undefined reference to `dm_usb_gadget_handle_interrupts'
>> make[1]: *** [Makefile:1793: u-boot] Error
>
> Ah! I guess this is caused by the "select DM_USB_GADGET" patch...
>
> You can revert it for now and I'll just select DM_USB_GADGET for qcom.

Will do, I'll drop it from the branch.

>
> It seems like there's a bunch going on around here with the cleanup by
> Marek. I can revisit this once that's settled

Yeah, we are working on reworking the interrupt handling a bit since it
has quite some limitations.

>> 
>> See: https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/jobs/802759#L470
>> 
>> I suspect that this is related to:
>> https://lore.kernel.org/r/all/20240225152715.1821613-1-jonas@kwiboo.se/
>
> Is this fixed by the ("usb: udc: dwc3: Fold board
> dm_usb_gadget_handle_interrupts() into DWC3") series?

Unfortunately, no:
https://source.denx.de/u-boot/custodians/u-boot-dfu/-/jobs/803228#L578

>> 
>> 
>>>
>>> Regards,
>>> Caleb
>>> ---
>>>  drivers/usb/gadget/f_mass_storage.c | 13 +++++++------
>>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
>>> index d880928044f4..ef90c7ec7fb5 100644
>>> --- a/drivers/usb/gadget/f_mass_storage.c
>>> +++ b/drivers/usb/gadget/f_mass_storage.c
>>> @@ -239,8 +239,9 @@
>>>  /* #define VERBOSE_DEBUG */
>>>  /* #define DUMP_MSGS */
>>>  
>>>  #include <config.h>
>>> +#include <div64.h>
>>>  #include <hexdump.h>
>>>  #include <log.h>
>>>  #include <malloc.h>
>>>  #include <common.h>
>>> @@ -768,10 +769,10 @@ static int do_read(struct fsg_common *common)
>>>  		}
>>>  
>>>  		/* Perform the read */
>>>  		rc = ums[common->lun].read_sector(&ums[common->lun],
>>> -				      file_offset / curlun->blksize,
>>> -				      amount / curlun->blksize,
>>> +				      lldiv(file_offset, curlun->blksize),
>>> +				      lldiv(amount, curlun->blksize),
>>>  				      (char __user *)bh->buf);
>>>  		if (!rc)
>>>  			return -EIO;
>>>  
>>> @@ -942,10 +943,10 @@ static int do_write(struct fsg_common *common)
>>>  			amount = bh->outreq->actual;
>>>  
>>>  			/* Perform the write */
>>>  			rc = ums[common->lun].write_sector(&ums[common->lun],
>>> -					       file_offset / curlun->blksize,
>>> -					       amount / curlun->blksize,
>>> +					       lldiv(file_offset, curlun->blksize),
>>> +					       lldiv(amount, curlun->blksize),
>>>  					       (char __user *)bh->buf);
>>>  			if (!rc)
>>>  				return -EIO;
>>>  			nwritten = rc * curlun->blksize;
>>> @@ -1058,10 +1059,10 @@ static int do_verify(struct fsg_common *common)
>>>  		}
>>>  
>>>  		/* Perform the read */
>>>  		rc = ums[common->lun].read_sector(&ums[common->lun],
>>> -				      file_offset / curlun->blksize,
>>> -				      amount / curlun->blksize,
>>> +				      lldiv(file_offset, curlun->blksize),
>>> +				      lldiv(amount, curlun->blksize),
>>>  				      (char __user *)bh->buf);
>>>  		if (!rc)
>>>  			return -EIO;
>>>  		nread = rc * curlun->blksize;
>>> -- 
>>> 2.44.0
>
> -- 
> // Caleb (they/them)