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
