[PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Abhishek Tamboli]

Change bl_len from u16 to u32 to accommodate the necessary bit shifts.

Fix the following smatch warnings:

drivers/usb/storage/ene_ub6250.c:1509 ms_scsi_read_capacity() warn: right shifting more than type allows 16 vs 24
drivers/usb/storage/ene_ub6250.c:1510 ms_scsi_read_capacity() warn: right shifting more than type allows 16 vs 16

Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
---
 drivers/usb/storage/ene_ub6250.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
index 97c66c0d91f4..ab6718dc874f 100644
--- a/drivers/usb/storage/ene_ub6250.c
+++ b/drivers/usb/storage/ene_ub6250.c
@@ -1484,7 +1484,7 @@ static int ms_scsi_mode_sense(struct us_data *us, struct scsi_cmnd *srb)
 static int ms_scsi_read_capacity(struct us_data *us, struct scsi_cmnd *srb)
 {
 	u32   bl_num;
-	u16    bl_len;
+	u32    bl_len;
 	unsigned int offset = 0;
 	unsigned char    buf[8];
 	struct scatterlist *sg = NULL;
-- 
2.34.1

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Alan Stern]

On Mon, Jul 29, 2024 at 11:53:48PM +0530, Abhishek Tamboli wrote:
> Change bl_len from u16 to u32 to accommodate the necessary bit shifts.
> 
> Fix the following smatch warnings:
> 
> drivers/usb/storage/ene_ub6250.c:1509 ms_scsi_read_capacity() warn: right shifting more than type allows 16 vs 24
> drivers/usb/storage/ene_ub6250.c:1510 ms_scsi_read_capacity() warn: right shifting more than type allows 16 vs 16
> 
> Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
> ---
>  drivers/usb/storage/ene_ub6250.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
> index 97c66c0d91f4..ab6718dc874f 100644
> --- a/drivers/usb/storage/ene_ub6250.c
> +++ b/drivers/usb/storage/ene_ub6250.c
> @@ -1484,7 +1484,7 @@ static int ms_scsi_mode_sense(struct us_data *us, struct scsi_cmnd *srb)
>  static int ms_scsi_read_capacity(struct us_data *us, struct scsi_cmnd *srb)
>  {
>  	u32   bl_num;
> -	u16    bl_len;
> +	u32    bl_len;
>  	unsigned int offset = 0;
>  	unsigned char    buf[8];
>  	struct scatterlist *sg = NULL;

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Oliver Neukum]

On 29.07.24 20:23, Abhishek Tamboli wrote:
> Change bl_len from u16 to u32 to accommodate the necessary bit shifts.

Hi,

while this patch is technically correct, it papers over the issue.
Could you please

1. use a constant, where a constant is used
2. use the macros for converting endianness

	Regards
		Oliver

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Abhishek Tamboli]

On Tue, Jul 30, 2024 at 09:09:05AM +0200, Oliver Neukum wrote:
> 
> 
> On 29.07.24 20:23, Abhishek Tamboli wrote:
> > Change bl_len from u16 to u32 to accommodate the necessary bit shifts.
> 
> Hi,
> 
> while this patch is technically correct, it papers over the issue.
> Could you please
Thank you for your feedback on my patch. I have a few questions to ensure 
I make the appropriate changes.
> 
> 1. use a constant, where a constant is used
I think you are suggesting that I should replace hard-coded values like the 
buffer size with named constants. For example:

#define BUF_SIZE 8
unsigned char buf[BUF_SIZE];

> 2. use the macros for converting endianness
Can I use macros like cpu_to_le32 for converting the bl_num and bl_len values.
Should I replace all instances of manual bitwise shifts with these macros?
For example:

    u32 bl_len = 0x200;
    buf[0] = cpu_to_le32(bl_num) >> 24;
    buf[4] = cpu_to_le32(bl_len) >> 24;

Is using cpu_to_le32 appropriate for the data format required by this
device?

I will make the necessary updates once I have your confirmation.

Best Regards,
Abhishek Tamboli
> 
> 	Regards
> 		Oliver
> 

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Oliver Neukum]

Hi,

On 30.07.24 19:56, Abhishek Tamboli wrote:
> On Tue, Jul 30, 2024 at 09:09:05AM +0200, Oliver Neukum wrote:

>> 1. use a constant, where a constant is used
> I think you are suggesting that I should replace hard-coded values like the
> buffer size with named constants. For example:
> 
> #define BUF_SIZE 8
> unsigned char buf[BUF_SIZE];

Yes, but the constant we need to look at here is bl_len.
This is a variable needlessly.

>> 2. use the macros for converting endianness
> Can I use macros like cpu_to_le32 for converting the bl_num and bl_len values.
> Should I replace all instances of manual bitwise shifts with these macros?

Yes.

> For example:
> 
>      u32 bl_len = 0x200;
>      buf[0] = cpu_to_le32(bl_num) >> 24;
>      buf[4] = cpu_to_le32(bl_len) >> 24;
> 
> Is using cpu_to_le32 appropriate for the data format required by this
> device?

Well, the format is big endian. So, cpu_to_be32() will be required.

	Regards
		Oliver

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Alan Stern]

On Wed, Jul 31, 2024 at 11:15:28AM +0200, 'Oliver Neukum' via USB Mass Storage on Linux wrote:
> Hi,
> 
> On 30.07.24 19:56, Abhishek Tamboli wrote:
> > On Tue, Jul 30, 2024 at 09:09:05AM +0200, Oliver Neukum wrote:
> 
> > > 1. use a constant, where a constant is used
> > I think you are suggesting that I should replace hard-coded values like the
> > buffer size with named constants. For example:
> > 
> > #define BUF_SIZE 8
> > unsigned char buf[BUF_SIZE];
> 
> Yes, but the constant we need to look at here is bl_len.
> This is a variable needlessly.

The code in ms_scsi_read_capacity() is written that way to be consistent 
with the sd_scsi_read_capacity() routine.  Or maybe it was just 
copied-and-pasted, and then the variable's type was changed for no good 
reason.

Replacing the variable with a constant won't make much difference.  The 
compiler will realize that bl_len has a constant value and will generate 
appropriate code anyway.  I think just changing the type is a fine fix.

> > > 2. use the macros for converting endianness
> > Can I use macros like cpu_to_le32 for converting the bl_num and bl_len values.
> > Should I replace all instances of manual bitwise shifts with these macros?
> 
> Yes.
> 
> > For example:
> > 
> >      u32 bl_len = 0x200;
> >      buf[0] = cpu_to_le32(bl_num) >> 24;
> >      buf[4] = cpu_to_le32(bl_len) >> 24;
> > 
> > Is using cpu_to_le32 appropriate for the data format required by this
> > device?
> 
> Well, the format is big endian. So, cpu_to_be32() will be required.

Better yet, use put_unaligned_be32().  However, there's nothing really 
wrong with the code as it stands.  It doesn't need to be changed now.

Alan Stern

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Abhishek Tamboli]

On Wed, Jul 31, 2024 at 10:04:33AM -0400, Alan Stern wrote:
> On Wed, Jul 31, 2024 at 11:15:28AM +0200, 'Oliver Neukum' via USB Mass Storage on Linux wrote:
> > Hi,
> > 
> > On 30.07.24 19:56, Abhishek Tamboli wrote:
> > > On Tue, Jul 30, 2024 at 09:09:05AM +0200, Oliver Neukum wrote:
> > 
> > > > 1. use a constant, where a constant is used
> > > I think you are suggesting that I should replace hard-coded values like the
> > > buffer size with named constants. For example:
> > > 
> > > #define BUF_SIZE 8
> > > unsigned char buf[BUF_SIZE];
> > 
> > Yes, but the constant we need to look at here is bl_len.
> > This is a variable needlessly.
> 
> The code in ms_scsi_read_capacity() is written that way to be consistent 
> with the sd_scsi_read_capacity() routine.  Or maybe it was just 
> copied-and-pasted, and then the variable's type was changed for no good 
> reason.
> 
> Replacing the variable with a constant won't make much difference.  The 
> compiler will realize that bl_len has a constant value and will generate 
> appropriate code anyway.  I think just changing the type is a fine fix.
> 
> > > > 2. use the macros for converting endianness
> > > Can I use macros like cpu_to_le32 for converting the bl_num and bl_len values.
> > > Should I replace all instances of manual bitwise shifts with these macros?
> > 
> > Yes.
> > 
> > > For example:
> > > 
> > >      u32 bl_len = 0x200;
> > >      buf[0] = cpu_to_le32(bl_num) >> 24;
> > >      buf[4] = cpu_to_le32(bl_len) >> 24;
> > > 
> > > Is using cpu_to_le32 appropriate for the data format required by this
> > > device?
> > 
> > Well, the format is big endian. So, cpu_to_be32() will be required.
> 
> Better yet, use put_unaligned_be32().
Would you recommend submitting a follow-up patch to incorporate this change, or should I leave it as is for now.
>However, there's nothing really 
>wrong with the code as it stands. It doesn't need to be changed now.
As you mentioned there's no need to change the code, So my initial patch is okay as is?

Thanks & Regards,
Abhishek Tamboli

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Alan Stern]

On Wed, Jul 31, 2024 at 11:34:45PM +0530, Abhishek Tamboli wrote:
> On Wed, Jul 31, 2024 at 10:04:33AM -0400, Alan Stern wrote:
> > On Wed, Jul 31, 2024 at 11:15:28AM +0200, 'Oliver Neukum' via USB Mass Storage on Linux wrote:
> > > Hi,
> > > 
> > > On 30.07.24 19:56, Abhishek Tamboli wrote:
> > > > On Tue, Jul 30, 2024 at 09:09:05AM +0200, Oliver Neukum wrote:
> > > 
> > > > > 1. use a constant, where a constant is used
> > > > I think you are suggesting that I should replace hard-coded values like the
> > > > buffer size with named constants. For example:
> > > > 
> > > > #define BUF_SIZE 8
> > > > unsigned char buf[BUF_SIZE];
> > > 
> > > Yes, but the constant we need to look at here is bl_len.
> > > This is a variable needlessly.
> > 
> > The code in ms_scsi_read_capacity() is written that way to be consistent 
> > with the sd_scsi_read_capacity() routine.  Or maybe it was just 
> > copied-and-pasted, and then the variable's type was changed for no good 
> > reason.
> > 
> > Replacing the variable with a constant won't make much difference.  The 
> > compiler will realize that bl_len has a constant value and will generate 
> > appropriate code anyway.  I think just changing the type is a fine fix.
> > 
> > > > > 2. use the macros for converting endianness
> > > > Can I use macros like cpu_to_le32 for converting the bl_num and bl_len values.
> > > > Should I replace all instances of manual bitwise shifts with these macros?
> > > 
> > > Yes.
> > > 
> > > > For example:
> > > > 
> > > >      u32 bl_len = 0x200;
> > > >      buf[0] = cpu_to_le32(bl_num) >> 24;
> > > >      buf[4] = cpu_to_le32(bl_len) >> 24;
> > > > 
> > > > Is using cpu_to_le32 appropriate for the data format required by this
> > > > device?
> > > 
> > > Well, the format is big endian. So, cpu_to_be32() will be required.
> > 
> > Better yet, use put_unaligned_be32().
> Would you recommend submitting a follow-up patch to incorporate this change, or should I leave it as is for now.

You can submit another patch as a clean-up, if you want.  But as I said, 
it isn't needed.

> >However, there's nothing really 
> >wrong with the code as it stands. It doesn't need to be changed now.
> As you mentioned there's no need to change the code, So my initial patch is okay as is?

It is as far as I'm concerned.  Obviously Oliver has a different 
opinion.  But I'm the Maintainer of the usb-storage driver, so my 
opinion counts for more than his does, in this case.  :-)

Alan Stern

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Abhishek Tamboli]

On Wed, Jul 31, 2024 at 02:19:54PM -0400, Alan Stern wrote:
> On Wed, Jul 31, 2024 at 11:34:45PM +0530, Abhishek Tamboli wrote:
> > On Wed, Jul 31, 2024 at 10:04:33AM -0400, Alan Stern wrote:
> > > On Wed, Jul 31, 2024 at 11:15:28AM +0200, 'Oliver Neukum' via USB Mass Storage on Linux wrote:
> > > > Hi,
> > > > 
> > > > On 30.07.24 19:56, Abhishek Tamboli wrote:
> > > > > On Tue, Jul 30, 2024 at 09:09:05AM +0200, Oliver Neukum wrote:
> > > > 
> > > > > > 1. use a constant, where a constant is used
> > > > > I think you are suggesting that I should replace hard-coded values like the
> > > > > buffer size with named constants. For example:
> > > > > 
> > > > > #define BUF_SIZE 8
> > > > > unsigned char buf[BUF_SIZE];
> > > > 
> > > > Yes, but the constant we need to look at here is bl_len.
> > > > This is a variable needlessly.
> > > 
> > > The code in ms_scsi_read_capacity() is written that way to be consistent 
> > > with the sd_scsi_read_capacity() routine.  Or maybe it was just 
> > > copied-and-pasted, and then the variable's type was changed for no good 
> > > reason.
> > > 
> > > Replacing the variable with a constant won't make much difference.  The 
> > > compiler will realize that bl_len has a constant value and will generate 
> > > appropriate code anyway.  I think just changing the type is a fine fix.
> > > 
> > > > > > 2. use the macros for converting endianness
> > > > > Can I use macros like cpu_to_le32 for converting the bl_num and bl_len values.
> > > > > Should I replace all instances of manual bitwise shifts with these macros?
> > > > 
> > > > Yes.
> > > > 
> > > > > For example:
> > > > > 
> > > > >      u32 bl_len = 0x200;
> > > > >      buf[0] = cpu_to_le32(bl_num) >> 24;
> > > > >      buf[4] = cpu_to_le32(bl_len) >> 24;
> > > > > 
> > > > > Is using cpu_to_le32 appropriate for the data format required by this
> > > > > device?
> > > > 
> > > > Well, the format is big endian. So, cpu_to_be32() will be required.
> > > 
> > > Better yet, use put_unaligned_be32().
> > Would you recommend submitting a follow-up patch to incorporate this change, or should I leave it as is for now.
> 
> You can submit another patch as a clean-up, if you want.  But as I said, 
> it isn't needed.
> 
> > >However, there's nothing really 
> > >wrong with the code as it stands. It doesn't need to be changed now.
> > As you mentioned there's no need to change the code, So my initial patch is okay as is?
> 
> It is as far as I'm concerned.  Obviously Oliver has a different 
> opinion.  But I'm the Maintainer of the usb-storage driver, so my 
> opinion counts for more than his does, in this case.  :-)
Thank you for your clarification and support. I appreciate your feedback.
I'm glad to know that my initial patch is acceptable to you.

Thanks & Regards
Abhishek Tamboli

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Oliver Neukum]

On 31.07.24 20:19, Alan Stern wrote:
> On Wed, Jul 31, 2024 at 11:34:45PM +0530, Abhishek Tamboli wrote:
>> On Wed, Jul 31, 2024 at 10:04:33AM -0400, Alan Stern wrote:

Hi,

I should make my reasoning clearer.

>>> Replacing the variable with a constant won't make much difference.  The
>>> compiler will realize that bl_len has a constant value and will generate
>>> appropriate code anyway.  I think just changing the type is a fine fix.

While that is absolutely true, it kind of removes the reason for the patch
in the first place. The code gcc generates is unlikely to be changed.

We are reacting to a warning an automatic tool generates. That is a good thing.
We should have clean code. The question is how we react to such a report.
It just seems to me that if we fix such a warning, the code should really be clean
after that. Just doing the minimum that will make the checker shut up is
no good.

	Regards
		Oliver

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Alan Stern]

On Thu, Aug 01, 2024 at 08:54:18AM +0200, 'Oliver Neukum' via USB Mass Storage on Linux wrote:
> 
> 
> On 31.07.24 20:19, Alan Stern wrote:
> > On Wed, Jul 31, 2024 at 11:34:45PM +0530, Abhishek Tamboli wrote:
> > > On Wed, Jul 31, 2024 at 10:04:33AM -0400, Alan Stern wrote:
> 
> Hi,
> 
> I should make my reasoning clearer.
> 
> > > > Replacing the variable with a constant won't make much difference.  The
> > > > compiler will realize that bl_len has a constant value and will generate
> > > > appropriate code anyway.  I think just changing the type is a fine fix.
> 
> While that is absolutely true, it kind of removes the reason for the patch
> in the first place. The code gcc generates is unlikely to be changed.
> 
> We are reacting to a warning an automatic tool generates. That is a good thing.
> We should have clean code. The question is how we react to such a report.
> It just seems to me that if we fix such a warning, the code should really be clean
> after that. Just doing the minimum that will make the checker shut up is
> no good.

With this fix, the code seems clean to me.  It may not be as short as 
possible, but it's clean.

Alan Stern

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Abhishek Tamboli]

Hi Alan,
On Thu, Aug 01, 2024 at 10:51:35AM -0400, Alan Stern wrote:
> On Thu, Aug 01, 2024 at 08:54:18AM +0200, 'Oliver Neukum' via USB Mass Storage on Linux wrote:
> > 
> > 
> > On 31.07.24 20:19, Alan Stern wrote:
> > > On Wed, Jul 31, 2024 at 11:34:45PM +0530, Abhishek Tamboli wrote:
> > > > On Wed, Jul 31, 2024 at 10:04:33AM -0400, Alan Stern wrote:
> > 
> > Hi,
> > 
> > I should make my reasoning clearer.
> > 
> > > > > Replacing the variable with a constant won't make much difference.  The
> > > > > compiler will realize that bl_len has a constant value and will generate
> > > > > appropriate code anyway.  I think just changing the type is a fine fix.
> > 
> > While that is absolutely true, it kind of removes the reason for the patch
> > in the first place. The code gcc generates is unlikely to be changed.
> > 
> > We are reacting to a warning an automatic tool generates. That is a good thing.
> > We should have clean code. The question is how we react to such a report.
> > It just seems to me that if we fix such a warning, the code should really be clean
> > after that. Just doing the minimum that will make the checker shut up is
> > no good.
> 
> With this fix, the code seems clean to me.  It may not be as short as 
> possible, but it's clean.

I noticed that the patch has not yet been pulled into linux-next, 
even though it has been acked-by you for over a month. Is there 
anything else I need to do on my end?

Thank you for your attention to this matter.

Regards,
Abhishek

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Alan Stern]

On Thu, Sep 12, 2024 at 06:15:18AM +0530, Abhishek Tamboli wrote:
> Hi Alan,

> I noticed that the patch has not yet been pulled into linux-next, 
> even though it has been acked-by you for over a month. Is there 
> anything else I need to do on my end?

I don't know what the story is.  Greg?

Alan Stern

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Greg KH]

On Thu, Sep 12, 2024 at 06:15:18AM +0530, Abhishek Tamboli wrote:
> Hi Alan,
> On Thu, Aug 01, 2024 at 10:51:35AM -0400, Alan Stern wrote:
> > On Thu, Aug 01, 2024 at 08:54:18AM +0200, 'Oliver Neukum' via USB Mass Storage on Linux wrote:
> > > 
> > > 
> > > On 31.07.24 20:19, Alan Stern wrote:
> > > > On Wed, Jul 31, 2024 at 11:34:45PM +0530, Abhishek Tamboli wrote:
> > > > > On Wed, Jul 31, 2024 at 10:04:33AM -0400, Alan Stern wrote:
> > > 
> > > Hi,
> > > 
> > > I should make my reasoning clearer.
> > > 
> > > > > > Replacing the variable with a constant won't make much difference.  The
> > > > > > compiler will realize that bl_len has a constant value and will generate
> > > > > > appropriate code anyway.  I think just changing the type is a fine fix.
> > > 
> > > While that is absolutely true, it kind of removes the reason for the patch
> > > in the first place. The code gcc generates is unlikely to be changed.
> > > 
> > > We are reacting to a warning an automatic tool generates. That is a good thing.
> > > We should have clean code. The question is how we react to such a report.
> > > It just seems to me that if we fix such a warning, the code should really be clean
> > > after that. Just doing the minimum that will make the checker shut up is
> > > no good.
> > 
> > With this fix, the code seems clean to me.  It may not be as short as 
> > possible, but it's clean.
> 
> I noticed that the patch has not yet been pulled into linux-next, 
> even though it has been acked-by you for over a month. Is there 
> anything else I need to do on my end?

Yes, please resend it, it is long gone from my review queue, sorry.

thanks,

greg k-h

Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

[Abhishek Tamboli]

On Thu, Sep 12, 2024 at 07:06:45AM +0200, Greg KH wrote:
> On Thu, Sep 12, 2024 at 06:15:18AM +0530, Abhishek Tamboli wrote:
> > Hi Alan,
> > On Thu, Aug 01, 2024 at 10:51:35AM -0400, Alan Stern wrote:
> > > On Thu, Aug 01, 2024 at 08:54:18AM +0200, 'Oliver Neukum' via USB Mass Storage on Linux wrote:
> > > > 
> > > > 
> > > > On 31.07.24 20:19, Alan Stern wrote:
> > > > > On Wed, Jul 31, 2024 at 11:34:45PM +0530, Abhishek Tamboli wrote:
> > > > > > On Wed, Jul 31, 2024 at 10:04:33AM -0400, Alan Stern wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > I should make my reasoning clearer.
> > > > 
> > > > > > > Replacing the variable with a constant won't make much difference.  The
> > > > > > > compiler will realize that bl_len has a constant value and will generate
> > > > > > > appropriate code anyway.  I think just changing the type is a fine fix.
> > > > 
> > > > While that is absolutely true, it kind of removes the reason for the patch
> > > > in the first place. The code gcc generates is unlikely to be changed.
> > > > 
> > > > We are reacting to a warning an automatic tool generates. That is a good thing.
> > > > We should have clean code. The question is how we react to such a report.
> > > > It just seems to me that if we fix such a warning, the code should really be clean
> > > > after that. Just doing the minimum that will make the checker shut up is
> > > > no good.
> > > 
> > > With this fix, the code seems clean to me.  It may not be as short as 
> > > possible, but it's clean.
> > 
> > I noticed that the patch has not yet been pulled into linux-next, 
> > even though it has been acked-by you for over a month. Is there 
> > anything else I need to do on my end?
> 
> Yes, please resend it, it is long gone from my review queue, sorry.
No problem, I will resend it.

Thanks,
Abhishek