| Message ID | 160763258826.26927.5141004289781904133.stgit@brunhilda (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | smartpqi updates | expand |
On Thu, 2020-12-10 at 14:36 -0600, Don Brace wrote: > From: Murthy Bhat <Murthy.Bhat@microchip.com> > > * Update enclosure identifier field corresponding to > physical devices in lsscsi/sysfs. > > Reviewed-by: Scott Benesh <scott.benesh@microchip.com> > Reviewed-by: Scott Teel <scott.teel@microchip.com> > Reviewed-by: Kevin Barnett <kevin.barnett@microchip.com> > Signed-off-by: Murthy Bhat <Murthy.Bhat@microchip.com> > Signed-off-by: Don Brace <don.brace@microchip.com> > --- > drivers/scsi/smartpqi/smartpqi_init.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c > b/drivers/scsi/smartpqi/smartpqi_init.c > index 1c51a59f1da6..40ae82470d8c 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -1841,7 +1841,6 @@ static void pqi_dev_info(struct pqi_ctrl_info > *ctrl_info, > static void pqi_scsi_update_device(struct pqi_scsi_dev > *existing_device, > struct pqi_scsi_dev *new_device) > { > - existing_device->devtype = new_device->devtype; > existing_device->device_type = new_device->device_type; > existing_device->bus = new_device->bus; > if (new_device->target_lun_valid) { > I don't get this. Why was it wrong to update the devtype field?
From: Martin Wilck [mailto:mwilck@suse.com] Subject: Re: [PATCH V3 22/25] smartpqi: update enclosure identifier in sysf > @@ -1841,7 +1841,6 @@ static void pqi_dev_info(struct pqi_ctrl_info > *ctrl_info, static void pqi_scsi_update_device(struct pqi_scsi_dev > *existing_device, > struct pqi_scsi_dev *new_device) { > - existing_device->devtype = new_device->devtype; > existing_device->device_type = new_device->device_type; > existing_device->bus = new_device->bus; > if (new_device->target_lun_valid) { > I don't get this. Why was it wrong to update the devtype field? Don: From patch Author... If we don't remove that statement, following issue will crop up. During initial device enumeration, the devtype attribute of the device(in current case enclosure device) is filled in slave_configure. But whenever a rescan occurs, the f/w would return zero for this field, and valid devtype is overwritten by zero, when device attributes are updated in pqi_scsi_update_device. Due to this lsscsi output shows wrong values.
On Mon, 2021-01-25 at 17:13 +0000, Don.Brace@microchip.com wrote: > > From: Martin Wilck [mailto:mwilck@suse.com] > Subject: Re: [PATCH V3 22/25] smartpqi: update enclosure identifier > in sysf > > > @@ -1841,7 +1841,6 @@ static void pqi_dev_info(struct pqi_ctrl_info > > *ctrl_info, static void pqi_scsi_update_device(struct pqi_scsi_dev > > *existing_device, > > struct pqi_scsi_dev *new_device) { > > - existing_device->devtype = new_device->devtype; > > existing_device->device_type = new_device->device_type; > > existing_device->bus = new_device->bus; > > if (new_device->target_lun_valid) { > > > > I don't get this. Why was it wrong to update the devtype field? > > Don: From patch Author... > If we don't remove that statement, following issue will crop up. > > During initial device enumeration, the devtype attribute of the > device(in current case enclosure device) is filled in > slave_configure. > But whenever a rescan occurs, the f/w would return zero for this > field, and valid devtype is overwritten by zero, when device > attributes are updated in pqi_scsi_update_device. Due to this lsscsi > output shows wrong values. Thanks. It would be very helpful for reviewers to add comments in cases like this. Regards, Martin > >
-----Original Message----- From: Martin Wilck [mailto:mwilck@suse.com] Subject: Re: [PATCH V3 22/25] smartpqi: update enclosure identifier in sysf > From: Martin Wilck [mailto:mwilck@suse.com] > Subject: Re: [PATCH V3 22/25] smartpqi: update enclosure identifier in > sysf > > > @@ -1841,7 +1841,6 @@ static void pqi_dev_info(struct pqi_ctrl_info > > *ctrl_info, static void pqi_scsi_update_device(struct pqi_scsi_dev > > *existing_device, > > struct pqi_scsi_dev *new_device) { > > - existing_device->devtype = new_device->devtype; > > existing_device->device_type = new_device->device_type; > > existing_device->bus = new_device->bus; > > if (new_device->target_lun_valid) { > > > > I don't get this. Why was it wrong to update the devtype field? > > Don: From patch Author... > If we don't remove that statement, following issue will crop up. > > During initial device enumeration, the devtype attribute of the > device(in current case enclosure device) is filled in slave_configure. > But whenever a rescan occurs, the f/w would return zero for this > field, and valid devtype is overwritten by zero, when device > attributes are updated in pqi_scsi_update_device. Due to this lsscsi > output shows wrong values. Thanks. It would be very helpful for reviewers to add comments in cases like this. Regards, Martin Don: I updated the patch description accordingly Thanks for all your hard-work. > >
diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c index 1c51a59f1da6..40ae82470d8c 100644 --- a/drivers/scsi/smartpqi/smartpqi_init.c +++ b/drivers/scsi/smartpqi/smartpqi_init.c @@ -1841,7 +1841,6 @@ static void pqi_dev_info(struct pqi_ctrl_info *ctrl_info, static void pqi_scsi_update_device(struct pqi_scsi_dev *existing_device, struct pqi_scsi_dev *new_device) { - existing_device->devtype = new_device->devtype; existing_device->device_type = new_device->device_type; existing_device->bus = new_device->bus; if (new_device->target_lun_valid) {