| Message ID | 20240729182348.451436-1-abhishektamboli9@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: storage: ene_ub6250: Fix right shift warnings | expand |
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>
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
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 >
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
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
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
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
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
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
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
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
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
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
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
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;
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(-)