| Message ID | 20220531175009.850-2-tyler.erickson@seagate.com |
|---|---|
| State | New |
| Headers | show |
| Series | ata,sd: Fix reading concurrent positioning ranges | expand |
On 6/1/22 02:50, Tyler Erickson wrote: > From: Tyler Erickson <tyler.j.erickson@seagate.com> > > The concurrent positioning ranges log is not a fixed size and may depend > on how many ranges are supported by the device. This patch uses the size > reported in the GPL directory to determine the number of pages supported > by the device before attempting to read this log page. > > Also fixing the page length in the SCSI translation for the concurrent > positioning ranges VPD page. > > This resolves this error from the dmesg output: > ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1 > > Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com> > Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com> > Tested-by: Michael English <michael.english@seagate.com> This needs a Fixes tag and cc stable. > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index ca64837641be..3d57fa84e2be 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -2003,16 +2003,16 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log, > return err_mask; > } > > -static bool ata_log_supported(struct ata_device *dev, u8 log) > +static int ata_log_supported(struct ata_device *dev, u8 log) > { > struct ata_port *ap = dev->link->ap; > > if (dev->horkage & ATA_HORKAGE_NO_LOG_DIR) > - return false; > + return 0; > > if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, ap->sector_buf, 1)) > - return false; > - return get_unaligned_le16(&ap->sector_buf[log * 2]) ? true : false; > + return 0; > + return get_unaligned_le16(&ap->sector_buf[log * 2]); > } > > static bool ata_identify_page_supported(struct ata_device *dev, u8 page) > @@ -2448,15 +2448,20 @@ static void ata_dev_config_cpr(struct ata_device *dev) > struct ata_cpr_log *cpr_log = NULL; > u8 *desc, *buf = NULL; > > - if (ata_id_major_version(dev->id) < 11 || > - !ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES)) > + if (ata_id_major_version(dev->id) < 11) > + goto out;> + > + buf_len = ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES); > + if (buf_len == 0) > goto out; > > /* > * Read the concurrent positioning ranges log (0x47). We can have at > - * most 255 32B range descriptors plus a 64B header. > + * most 255 32B range descriptors plus a 64B header. This log varies in > + * size, so use the size reported in the GPL directory. Reading beyond > + * the supported length will result in an error. > */ > - buf_len = (64 + 255 * 32 + 511) & ~511; > + buf_len <<= 9; > buf = kzalloc(buf_len, GFP_KERNEL); > if (!buf) > goto out; > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 06c9d90238d9..0ea9c3115529 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -2101,7 +2101,7 @@ static unsigned int ata_scsiop_inq_b9(struct ata_scsi_args *args, u8 *rbuf) > > /* SCSI Concurrent Positioning Ranges VPD page: SBC-5 rev 1 or later */ > rbuf[1] = 0xb9; > - put_unaligned_be16(64 + (int)cpr_log->nr_cpr * 32 - 4, &rbuf[3]); > + put_unaligned_be16(64 + (int)cpr_log->nr_cpr * 32 - 4, &rbuf[2]); > > for (i = 0; i < cpr_log->nr_cpr; i++, desc += 32) { > desc[0] = cpr_log->cpr[i].num; This scsi hunk needs to be a different patch with a fixes tag and cc stable.
Hello! On 5/31/22 8:50 PM, Tyler Erickson wrote: > From: Tyler Erickson <tyler.j.erickson@seagate.com> > > The concurrent positioning ranges log is not a fixed size and may depend > on how many ranges are supported by the device. This patch uses the size > reported in the GPL directory to determine the number of pages supported > by the device before attempting to read this log page. > > Also fixing the page length in the SCSI translation for the concurrent > positioning ranges VPD page. > > This resolves this error from the dmesg output: > ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1 > > Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com> > Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com> > Tested-by: Michael English <michael.english@seagate.com> > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index ca64837641be..3d57fa84e2be 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -2003,16 +2003,16 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log, > return err_mask; > } > > -static bool ata_log_supported(struct ata_device *dev, u8 log) > +static int ata_log_supported(struct ata_device *dev, u8 log) Maybe *unsigned int*? The 'buf_len' variable below is 'size_t' which is *unsigned* type... > { > struct ata_port *ap = dev->link->ap; > > if (dev->horkage & ATA_HORKAGE_NO_LOG_DIR) > - return false; > + return 0; > > if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, ap->sector_buf, 1)) > - return false; > - return get_unaligned_le16(&ap->sector_buf[log * 2]) ? true : false; > + return 0; > + return get_unaligned_le16(&ap->sector_buf[log * 2]); > } > > static bool ata_identify_page_supported(struct ata_device *dev, u8 page) > @@ -2448,15 +2448,20 @@ static void ata_dev_config_cpr(struct ata_device *dev) > struct ata_cpr_log *cpr_log = NULL; > u8 *desc, *buf = NULL; > > - if (ata_id_major_version(dev->id) < 11 || > - !ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES)) > + if (ata_id_major_version(dev->id) < 11) > + goto out; > + > + buf_len = ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES); > + if (buf_len == 0) > goto out; [...] MBR, Sergey
On 2022/06/01 19:29, Sergey Shtylyov wrote: > Hello! > > On 5/31/22 8:50 PM, Tyler Erickson wrote: > >> From: Tyler Erickson <tyler.j.erickson@seagate.com> >> >> The concurrent positioning ranges log is not a fixed size and may depend >> on how many ranges are supported by the device. This patch uses the size >> reported in the GPL directory to determine the number of pages supported >> by the device before attempting to read this log page. >> >> Also fixing the page length in the SCSI translation for the concurrent >> positioning ranges VPD page. >> >> This resolves this error from the dmesg output: >> ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1 >> >> Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com> >> Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com> >> Tested-by: Michael English <michael.english@seagate.com> >> >> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >> index ca64837641be..3d57fa84e2be 100644 >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c >> @@ -2003,16 +2003,16 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log, >> return err_mask; >> } >> >> -static bool ata_log_supported(struct ata_device *dev, u8 log) >> +static int ata_log_supported(struct ata_device *dev, u8 log) > > Maybe *unsigned int*? The 'buf_len' variable below is 'size_t' which is *unsigned* type... int is fine I think. The value is 16-bits so no overflow possible. And having a signed value, we can change the code to return an error code if ever needed. > >> { >> struct ata_port *ap = dev->link->ap; >> >> if (dev->horkage & ATA_HORKAGE_NO_LOG_DIR) >> - return false; >> + return 0; >> >> if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, ap->sector_buf, 1)) >> - return false; >> - return get_unaligned_le16(&ap->sector_buf[log * 2]) ? true : false; >> + return 0; >> + return get_unaligned_le16(&ap->sector_buf[log * 2]); >> } >> >> static bool ata_identify_page_supported(struct ata_device *dev, u8 page) >> @@ -2448,15 +2448,20 @@ static void ata_dev_config_cpr(struct ata_device *dev) >> struct ata_cpr_log *cpr_log = NULL; >> u8 *desc, *buf = NULL; >> >> - if (ata_id_major_version(dev->id) < 11 || >> - !ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES)) >> + if (ata_id_major_version(dev->id) < 11) >> + goto out; >> + >> + buf_len = ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES); >> + if (buf_len == 0) >> goto out; > [...] > > MBR, Sergey
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index ca64837641be..3d57fa84e2be 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -2003,16 +2003,16 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log, return err_mask; } -static bool ata_log_supported(struct ata_device *dev, u8 log) +static int ata_log_supported(struct ata_device *dev, u8 log) { struct ata_port *ap = dev->link->ap; if (dev->horkage & ATA_HORKAGE_NO_LOG_DIR) - return false; + return 0; if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, ap->sector_buf, 1)) - return false; - return get_unaligned_le16(&ap->sector_buf[log * 2]) ? true : false; + return 0; + return get_unaligned_le16(&ap->sector_buf[log * 2]); } static bool ata_identify_page_supported(struct ata_device *dev, u8 page) @@ -2448,15 +2448,20 @@ static void ata_dev_config_cpr(struct ata_device *dev) struct ata_cpr_log *cpr_log = NULL; u8 *desc, *buf = NULL; - if (ata_id_major_version(dev->id) < 11 || - !ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES)) + if (ata_id_major_version(dev->id) < 11) + goto out; + + buf_len = ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES); + if (buf_len == 0) goto out; /* * Read the concurrent positioning ranges log (0x47). We can have at - * most 255 32B range descriptors plus a 64B header. + * most 255 32B range descriptors plus a 64B header. This log varies in + * size, so use the size reported in the GPL directory. Reading beyond + * the supported length will result in an error. */ - buf_len = (64 + 255 * 32 + 511) & ~511; + buf_len <<= 9; buf = kzalloc(buf_len, GFP_KERNEL); if (!buf) goto out; diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 06c9d90238d9..0ea9c3115529 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -2101,7 +2101,7 @@ static unsigned int ata_scsiop_inq_b9(struct ata_scsi_args *args, u8 *rbuf) /* SCSI Concurrent Positioning Ranges VPD page: SBC-5 rev 1 or later */ rbuf[1] = 0xb9; - put_unaligned_be16(64 + (int)cpr_log->nr_cpr * 32 - 4, &rbuf[3]); + put_unaligned_be16(64 + (int)cpr_log->nr_cpr * 32 - 4, &rbuf[2]); for (i = 0; i < cpr_log->nr_cpr; i++, desc += 32) { desc[0] = cpr_log->cpr[i].num;