Re: [PATCH] usb: gadget: UMS: fix 64-bit division on ARM32
Hi Caleb,
On jeu., mars 21, 2024 at 17:01, Caleb Connolly
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
>> 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
>>
>> Reviewed-by: Mattijs Korpershoek
>>
>> 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-jo...@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
>>> +#include
>>> #include
>>> #include
>>> #include
>>> #include
>>> @@ -768,10 +769,10 @@ static int do_read(struct fsg_common *common)
>>> }
>>>
>>> /* Perform the read */
>>> rc = ums[common->lun].read_sector([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([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([common->lun],
>>> - file_offset / curlun->blksize,
>>> - amount / curlun->blksize,
>>> + lldiv(file_offset, curlun->blksize),
Re: [PATCH] usb: gadget: UMS: fix 64-bit division on ARM32
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
> 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
>
> Reviewed-by: Mattijs Korpershoek
>
> 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.
>
>
>> ---
>> 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.
It seems like there's a bunch going on around here with the cleanup by
Marek. I can revisit this once that's settled
>
> 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-jo...@kwiboo.se/
Is this fixed by the ("usb: udc: dwc3: Fold board
dm_usb_gadget_handle_interrupts() into DWC3") series?
>
>
>>
>> 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
>> +#include
>> #include
>> #include
>> #include
>> #include
>> @@ -768,10 +769,10 @@ static int do_read(struct fsg_common *common)
>> }
>>
>> /* Perform the read */
>> rc = ums[common->lun].read_sector([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([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([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)
Re: [PATCH] usb: gadget: UMS: fix 64-bit division on ARM32
Hi Caleb,
Thank you for the patch.
On jeu., mars 21, 2024 at 15:28, Caleb Connolly
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
Reviewed-by: Mattijs Korpershoek
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.
> ---
> 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
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-jo...@kwiboo.se/
>
> 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
> +#include
> #include
> #include
> #include
> #include
> @@ -768,10 +769,10 @@ static int do_read(struct fsg_common *common)
> }
>
> /* Perform the read */
> rc = ums[common->lun].read_sector([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([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([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
