| Message ID | 20240525063653.2331587-1-shichaorai@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] usb-storage: Check whether the media is initialized successfully | expand |
On Sat, May 25, 2024 at 02:36:53PM +0800, Shichao Lai wrote: > The member "uzonesize" of struct alauda_info will remain 0 > if alauda_init_media() fails, potentially causing divide errors > in alauda_read_data() and alauda_write_lba(). > - Add a member "initialized" to struct alauda_info as a symbol > for media initialization. > - Change a condition in alauda_check_media() to ensure the > first initialization. > - Add an error check for the return value of alauda_init_media(). > > Reported-by: xingwei lee <xrivendell7@gmail.com> > Reported-by: yue sun <samsun1006219@gmail.com> > Suggested-by: Oliver Neukum <oneukum@suse.com> Oliver did not suggest that the patch be written this way. > Signed-off-by: Shichao Lai <shichaorai@gmail.com> > --- > Changes since v1: > - Check the initialization of alauda_check_media() > which is the root cause. > > drivers/usb/storage/alauda.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c > index 115f05a6201a..ddf0da203481 100644 > --- a/drivers/usb/storage/alauda.c > +++ b/drivers/usb/storage/alauda.c > @@ -105,6 +105,8 @@ struct alauda_info { > unsigned char sense_key; > unsigned long sense_asc; /* additional sense code */ > unsigned long sense_ascq; /* additional sense code qualifier */ > + > + bool initialized; /* true if the media is initialized */ Now with the patch written out, I think a better name for this variable would be media_initialized. That is a better description of what it means (it doesn't mean that the driver or the device is initialized). And then you could remove the comment, because it would be obvious. > }; > > #define short_pack(lsb,msb) ( ((u16)(lsb)) | ( ((u16)(msb))<<8 ) ) > @@ -476,11 +478,12 @@ static int alauda_check_media(struct us_data *us) > } > > /* Check for media change */ > - if (status[0] & 0x08) { > + if (status[0] & 0x08 || !info->initialized) { > usb_stor_dbg(us, "Media change detected\n"); > alauda_free_maps(&MEDIA_INFO(us)); > - alauda_init_media(us); > - > + rc = alauda_init_media(us); > + if (rc == USB_STOR_TRANSPORT_GOOD) > + info->initialized = true; > info->sense_key = UNIT_ATTENTION; > info->sense_asc = 0x28; > info->sense_ascq = 0x00; > @@ -1120,6 +1123,7 @@ static int init_alauda(struct us_data *us) > info->wr_ep = usb_sndbulkpipe(us->pusb_dev, > altsetting->endpoint[0].desc.bEndpointAddress > & USB_ENDPOINT_NUMBER_MASK); > + info->initialized = false; You don't need to do this. The info pointer is an alias for us->extra, which is allocated by kzalloc(), which clears all the memory it allocates to zero. Alan Stern
> Oliver did not suggest that the patch be written this way. > > Now with the patch written out, I think a better name for this variable > would be media_initialized. That is a better description of what it > means (it doesn't mean that the driver or the device is initialized). > And then you could remove the comment, because it would be obvious. > > You don't need to do this. The info pointer is an alias for us->extra, > which is allocated by kzalloc(), which clears all the memory it > allocates to zero. > > Alan Stern Thanks for your patient feedback! I will rewrite the patch soon.
diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c index 115f05a6201a..ddf0da203481 100644 --- a/drivers/usb/storage/alauda.c +++ b/drivers/usb/storage/alauda.c @@ -105,6 +105,8 @@ struct alauda_info { unsigned char sense_key; unsigned long sense_asc; /* additional sense code */ unsigned long sense_ascq; /* additional sense code qualifier */ + + bool initialized; /* true if the media is initialized */ }; #define short_pack(lsb,msb) ( ((u16)(lsb)) | ( ((u16)(msb))<<8 ) ) @@ -476,11 +478,12 @@ static int alauda_check_media(struct us_data *us) } /* Check for media change */ - if (status[0] & 0x08) { + if (status[0] & 0x08 || !info->initialized) { usb_stor_dbg(us, "Media change detected\n"); alauda_free_maps(&MEDIA_INFO(us)); - alauda_init_media(us); - + rc = alauda_init_media(us); + if (rc == USB_STOR_TRANSPORT_GOOD) + info->initialized = true; info->sense_key = UNIT_ATTENTION; info->sense_asc = 0x28; info->sense_ascq = 0x00; @@ -1120,6 +1123,7 @@ static int init_alauda(struct us_data *us) info->wr_ep = usb_sndbulkpipe(us->pusb_dev, altsetting->endpoint[0].desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK); + info->initialized = false; return 0; }
The member "uzonesize" of struct alauda_info will remain 0 if alauda_init_media() fails, potentially causing divide errors in alauda_read_data() and alauda_write_lba(). - Add a member "initialized" to struct alauda_info as a symbol for media initialization. - Change a condition in alauda_check_media() to ensure the first initialization. - Add an error check for the return value of alauda_init_media(). Reported-by: xingwei lee <xrivendell7@gmail.com> Reported-by: yue sun <samsun1006219@gmail.com> Suggested-by: Oliver Neukum <oneukum@suse.com> Signed-off-by: Shichao Lai <shichaorai@gmail.com> --- Changes since v1: - Check the initialization of alauda_check_media() which is the root cause. drivers/usb/storage/alauda.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)