Re: [PATCH 6/6] Invalidate VPD page data
On Sat, 2014-03-15 at 09:51 +0100, Hannes Reinecke wrote:
> Add a flag 'vpd_invalid' to the SCSI device to indicate that
> the VPD data needs to be refreshed. This is required if either
> a manual rescan is triggered or if the sense code INQUIRY DATA
> HAS CHANGED has been received.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/scsi.c| 91
> ++
> drivers/scsi/scsi_error.c | 1 +
> drivers/scsi/scsi_scan.c | 7 +++-
> drivers/scsi/scsi_sysfs.c | 6 ++-
> drivers/scsi/ses.c | 2 +-
> include/scsi/scsi_device.h | 2 +
> 6 files changed, 82 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 2669cb8..971b099 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -1056,10 +1056,11 @@ void scsi_attach_vpd(struct scsi_device *sdev)
> int vpd_len = 255;
> int pg80_supported = 0;
> int pg83_supported = 0;
> - unsigned char *vpd_buf;
> + unsigned char *vpd_buf, *tmp_pg;
>
> if (sdev->skip_vpd_pages)
> return;
> +
> retry_pg0:
> vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> if (!vpd_buf)
> @@ -1087,45 +1088,89 @@ retry_pg0:
> }
> kfree(vpd_buf);
>
> - if (pg80_supported) {
> retry_pg80:
> + if (pg80_supported) {
> vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> if (!vpd_buf)
> - return;
> -
> - result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
> + result = -ENOMEM;
> + else
> + result = scsi_vpd_inquiry(sdev, vpd_buf,
> + 0x80, vpd_len);
> if (result < 0) {
> kfree(vpd_buf);
> + spin_lock(&sdev->reconfig_lock);
> + tmp_pg = sdev->vpd_pg80;
> + sdev->vpd_pg80 = NULL;
> + sdev->vpd_pg80_len = result;
> + kfree(tmp_pg);
> + spin_unlock(&sdev->reconfig_lock);
> + /*
> + * An unexpected error occurred,
> + * do not clear vpd_invalid flag
> + */
> return;
> + } else {
> + if (result > vpd_len) {
> + vpd_len = result;
> + kfree(vpd_buf);
> + goto retry_pg80;
> + }
> + spin_lock(&sdev->reconfig_lock);
> + sdev->vpd_pg80 = vpd_buf;
> + sdev->vpd_pg80_len = result;
> + spin_unlock(&sdev->reconfig_lock);
> }
> - if (result > vpd_len) {
> - vpd_len = result;
> - kfree(vpd_buf);
> - goto retry_pg80;
> - }
> - sdev->vpd_pg80_len = result;
> - sdev->vpd_pg80 = vpd_buf;
> + } else {
> + spin_lock(&sdev->reconfig_lock);
> + tmp_pg = sdev->vpd_pg80;
> + sdev->vpd_pg80 = NULL;
> + sdev->vpd_pg80_len = -ENOENT;
> + kfree(tmp_pg);
> + spin_unlock(&sdev->reconfig_lock);
Actually, this reconfig lock thing doesn't work. If you look in ses,
we're looping over the vpd_lengths and taking offsets into the data
while the data is changing, regardless of the lock.
Firstly, you can just do a kfree on sdev->vpd_pgxx; no need for the
tmp_pg. Secondly, we would now need a helper to return the vpd_pgxx
which sees the lock and probably copies the data to make sure we can't
get a change while using the data.
I've dropped this patch for now.
James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 6/6] Invalidate VPD page data
> On 03/17/2014 11:11 PM, Jeremy Linton wrote: > > > > I didn't study the whole code path but does the VPD data get > > updated on a 6/2900? I suspect it should be. I can imagine a > > number of cases where the luns changed check condition gets > > preempted/lost by a device reset. I guess much of that should > > be masked by the port login/logout, but its probably better to > > be safe... > > See the unit attention condition precedence level table in the SCSI Architecture Model (sam5r15 table 54 page 121). POWER ON, RESET, OR BUS DEVICE RESET OCCURRED (29h 00h) has the highest precedence, meaning that any other unit attention condition might also have occurred but no longer be individually reported. Similarly, all the others in table 54 have precedence over (and thus imply the possibility of) INQUIRY DATA HAS CHANGED. In addition to the codes in table 54, since INQUIRY DATA HAS CHANGED has a nonzero ASCQ (3Fh 03h), it also has lower precedence than 3Fh 00h TARGET OPERATING CONDITIONS HAVE CHANGED. For generic SCSI code, the "protocol specific" codes may be the hardest to handle. That covers these old parallel SCSI codes: 29h 05h TRANSCEIVER MODE CHANGED TO SINGLE-ENDED 29h 06h TRANSCEIVER MODE CHANGED TO LVD but allows newer protocols to add codes if needed. You may need the LLD to report an optional list of extra codes to consider at that precedence level. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] Invalidate VPD page data
On Tue, 2014-03-18 at 07:52 +0100, Hannes Reinecke wrote:
> On 03/17/2014 11:11 PM, Jeremy Linton wrote:
> > On 3/15/2014 3:51 AM, Hannes Reinecke wrote:
> >> Add a flag 'vpd_invalid' to the SCSI device to indicate that
> >> the VPD data needs to be refreshed. This is required if
> >> either a manual rescan is triggered or if the sense code
> >> INQUIRY DATA HAS CHANGED has been received.
> >
> >
> >> --- a/drivers/scsi/scsi_error.c +++
> >> b/drivers/scsi/scsi_error.c @@ -393,6 +393,7 @@ static void
> >> scsi_report_sense(struct scsi_device *sdev,
> >
> >> if (sshdr->sense_key == UNIT_ATTENTION) { if (sshdr->asc ==
> >> 0x3f && sshdr->ascq == 0x03) { + sdev->vpd_invalid = 1;
> >
> >
> > I didn't study the whole code path but does the VPD data get
> > updated on a 6/2900? I suspect it should be. I can imagine a
> > number of cases where the luns changed check condition gets
> > preempted/lost by a device reset. I guess much of that should
> > be masked by the port login/logout, but its probably better to
> > be safe...
> >
> Argl.
>
> I was hoping to avoid that; I've already had a rather lengthy
> discussion with NetApp about handling Power-on-Reset UA.
>
> We should be discussion that at LSF; Power-on-Reset UA handling
> (and queued UA handling in general) has some implications which
> could do with a proper elaboration. Rumours have it that Fred
> Knight from NetApp will also at LSF, so we'll have someone to ask
> for any technical issues :-).
You don't have to rely on rumour; the attendee list is online:
https://docs.google.com/spreadsheet/pub?key=0ArurRVMVCSnkdHU2Zk1KbFhmeVZFVmFMQ19nakJYaFE&gid=1
I'm not sure there's a full session on power on/reset UA handling.
Right at the moment, we eat any UA after a reset. Perhaps we should
collect the sense and dump it if it's what we expect.
James
> And that's precisely why I hooked the 'sdev->vpd_invalid' flag
> into the 'rescan' attribute, so that it can be refreshed on demand.
>
> Cheers,
>
> Hannes
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] Invalidate VPD page data
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 03/17/2014 11:11 PM, Jeremy Linton wrote:
> On 3/15/2014 3:51 AM, Hannes Reinecke wrote:
>> Add a flag 'vpd_invalid' to the SCSI device to indicate that
>> the VPD data needs to be refreshed. This is required if
>> either a manual rescan is triggered or if the sense code
>> INQUIRY DATA HAS CHANGED has been received.
>
>
>> --- a/drivers/scsi/scsi_error.c +++
>> b/drivers/scsi/scsi_error.c @@ -393,6 +393,7 @@ static void
>> scsi_report_sense(struct scsi_device *sdev,
>
>> if (sshdr->sense_key == UNIT_ATTENTION) { if (sshdr->asc ==
>> 0x3f && sshdr->ascq == 0x03) { + sdev->vpd_invalid = 1;
>
>
> I didn't study the whole code path but does the VPD data get
> updated on a 6/2900? I suspect it should be. I can imagine a
> number of cases where the luns changed check condition gets
> preempted/lost by a device reset. I guess much of that should
> be masked by the port login/logout, but its probably better to
> be safe...
>
Argl.
I was hoping to avoid that; I've already had a rather lengthy
discussion with NetApp about handling Power-on-Reset UA.
We should be discussion that at LSF; Power-on-Reset UA handling
(and queued UA handling in general) has some implications which
could do with a proper elaboration. Rumours have it that Fred
Knight from NetApp will also at LSF, so we'll have someone to ask
for any technical issues :-).
And that's precisely why I hooked the 'sdev->vpd_invalid' flag
into the 'rescan' attribute, so that it can be refreshed on demand.
Cheers,
Hannes
- --
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJTJ+1KAAoJEGz4yi9OyKjP8G8P/2+fO+nYNCJa9KyMfx9/IUke
VA5Ap81H2tbyUkmZp9irnegV9HNg7OF66MMNo+o5MvB96RZGCnma5pjUfmwpk4AW
as188YlrT36FSC769LH+7n7EA5rcCVuCzRkZkiqjvh7xG36Z7yg4HpKjx1aX0tHh
3iTvYy1IuPfhzti4W533lybmrFYJi7Izjr/qHV/AjeWlkmsGSVf/eb4A84dKskjn
RB0ahFrS86HVCOOvH233tsjNrN1ToP7+nNQT4cyDGT1mUqQZHQlNCz75zhJBUjAL
QuT31T2SiT0dS3EmpLIU/oI0A0rN8NbJo7zV1LpbVTHvJCVrNtkUtjMtxFV0/CMk
/ppWZGyIRjkPcCzNS01QVI1bUywpPbOhfVjJiG15gYY1Ef6z6/uktuX9SNOGWAzw
1MYylda+c10rg3I47nZTjahTKnnGkKBi7OLVXqi0hs0lHW7gA0UpZLYkovcOL4bA
7fYwABVDmfP65bzWl1tk9qmWoLeneD9TW5aaLkQ8d8qr/pd3oo1zd+RC6cgBQVXH
A2tRHTF36+R7C8kOO937tvQ2QIWa5YXNlgeaQkYAoOegJq0CWTB2kSe+f/h9EBtv
+YNLNPldhHD3bj0Poqrw2wXVfEWFlXD5Blz/2ioHPzzOx6EkQgH5uNGzyuyKR+5p
MMNGFIoQZSyqfJJNVtIK
=0T1G
-END PGP SIGNATURE-
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] Invalidate VPD page data
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 3/15/2014 3:51 AM, Hannes Reinecke wrote:
> Add a flag 'vpd_invalid' to the SCSI device to indicate that the VPD data
> needs to be refreshed. This is required if either a manual rescan is
> triggered or if the sense code INQUIRY DATA HAS CHANGED has been received.
> --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -393,6
> +393,7 @@ static void scsi_report_sense(struct scsi_device *sdev,
>
> if (sshdr->sense_key == UNIT_ATTENTION) { if (sshdr->asc == 0x3f &&
> sshdr->ascq == 0x03) { + sdev->vpd_invalid = 1;
I didn't study the whole code path but does the VPD data get updated on
a
6/2900? I suspect it should be.
I can imagine a number of cases where the luns changed check condition
gets
preempted/lost by a device reset. I guess much of that should be masked by the
port login/logout, but its probably better to be safe...
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQEcBAEBAgAGBQJTJ3MRAAoJEL5i86xrzcy7vsAIAKyiMPZ0FBlLRxlQsGQxHaet
8FTCoj0GtgE1hmw+BfLvKzdR5VqMNt/yTSsJd/8OZrykDQ298TQlfgoSle7/dpYp
FDaMq2uXINGpe+EC/OvVGH8GJbOgdjLectwu2EqKhkMblpyBPM83XXWNOD1lbLYf
/TN/WPug9s5NOwdwSxeNhZRZKVw/9T33fxVKlXQg/sExfjIeFqHSTxIRH9bvktvw
/ewe85P8WNtTXwZUGj1O3PaPzg0B98+LgHmAJNYREBf7t/mDZpkR492Ty9fRKkxi
SauSIvdaNWuc28a88xaJGD+WRDPqSbLjecpNnWiYNfbNrNKx/WoJUpfVJS+Ltmk=
=mfSZ
-END PGP SIGNATURE-
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/6] Invalidate VPD page data
Add a flag 'vpd_invalid' to the SCSI device to indicate that
the VPD data needs to be refreshed. This is required if either
a manual rescan is triggered or if the sense code INQUIRY DATA
HAS CHANGED has been received.
Signed-off-by: Hannes Reinecke
---
drivers/scsi/scsi.c| 91 ++
drivers/scsi/scsi_error.c | 1 +
drivers/scsi/scsi_scan.c | 7 +++-
drivers/scsi/scsi_sysfs.c | 6 ++-
drivers/scsi/ses.c | 2 +-
include/scsi/scsi_device.h | 2 +
6 files changed, 82 insertions(+), 27 deletions(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2669cb8..971b099 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -1056,10 +1056,11 @@ void scsi_attach_vpd(struct scsi_device *sdev)
int vpd_len = 255;
int pg80_supported = 0;
int pg83_supported = 0;
- unsigned char *vpd_buf;
+ unsigned char *vpd_buf, *tmp_pg;
if (sdev->skip_vpd_pages)
return;
+
retry_pg0:
vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
if (!vpd_buf)
@@ -1087,45 +1088,89 @@ retry_pg0:
}
kfree(vpd_buf);
- if (pg80_supported) {
retry_pg80:
+ if (pg80_supported) {
vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
if (!vpd_buf)
- return;
-
- result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
+ result = -ENOMEM;
+ else
+ result = scsi_vpd_inquiry(sdev, vpd_buf,
+ 0x80, vpd_len);
if (result < 0) {
kfree(vpd_buf);
+ spin_lock(&sdev->reconfig_lock);
+ tmp_pg = sdev->vpd_pg80;
+ sdev->vpd_pg80 = NULL;
+ sdev->vpd_pg80_len = result;
+ kfree(tmp_pg);
+ spin_unlock(&sdev->reconfig_lock);
+ /*
+* An unexpected error occurred,
+* do not clear vpd_invalid flag
+*/
return;
+ } else {
+ if (result > vpd_len) {
+ vpd_len = result;
+ kfree(vpd_buf);
+ goto retry_pg80;
+ }
+ spin_lock(&sdev->reconfig_lock);
+ sdev->vpd_pg80 = vpd_buf;
+ sdev->vpd_pg80_len = result;
+ spin_unlock(&sdev->reconfig_lock);
}
- if (result > vpd_len) {
- vpd_len = result;
- kfree(vpd_buf);
- goto retry_pg80;
- }
- sdev->vpd_pg80_len = result;
- sdev->vpd_pg80 = vpd_buf;
+ } else {
+ spin_lock(&sdev->reconfig_lock);
+ tmp_pg = sdev->vpd_pg80;
+ sdev->vpd_pg80 = NULL;
+ sdev->vpd_pg80_len = -ENOENT;
+ kfree(tmp_pg);
+ spin_unlock(&sdev->reconfig_lock);
}
- if (pg83_supported) {
retry_pg83:
+ if (pg83_supported) {
vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
if (!vpd_buf)
- return;
-
- result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
+ result = -ENOMEM;
+ else
+ result = scsi_vpd_inquiry(sdev, vpd_buf,
+ 0x83, vpd_len);
if (result < 0) {
kfree(vpd_buf);
+ spin_lock(&sdev->reconfig_lock);
+ tmp_pg = sdev->vpd_pg83;
+ sdev->vpd_pg83 = NULL;
+ sdev->vpd_pg83_len = result;
+ kfree(tmp_pg);
+ spin_unlock(&sdev->reconfig_lock);
+ /*
+* An unexpected error occurred,
+* do not clear vpd_invalid flag
+*/
return;
+ } else {
+ if (result > vpd_len) {
+ vpd_len = result;
+ kfree(vpd_buf);
+ goto retry_pg83;
+ }
+ spin_lock(&sdev->reconfig_lock);
+ sdev->vpd_pg83 = vpd_buf;
+ sdev->vpd_pg83_len = result;
+ spin_unlock(&sdev->reconfig_lock);
}
- if (result > vpd_len) {
- vpd_len = result;
- kfree(vpd_buf);
- goto retry_pg83;
- }
- sdev->vpd_pg83_len = result;
- sde
