On 07/24/2015 04:38 PM, Christoph Hellwig wrote:
> On Wed, Jul 08, 2015 at 01:09:44PM +0200, Hannes Reinecke wrote:
>> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>> @@ -264,8 +264,11 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
>> int device_id_size, device_id_type = 0;
>> struct alua_port_group *tmp_pg, *pg = NULL;
>>
>> - if (!sdev->vpd_pg83)
>> + rcu_read_lock();
>> + if (!rcu_dereference(sdev->vpd_pg83)) {
>> + rcu_read_unlock();
>> return SCSI_DH_DEV_UNSUPP;
>> + }
>>
>> /*
>> * Look for the correct descriptor.
>> @@ -281,8 +284,8 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
>> */
>> memset(device_id_str, 0, 256);
>> device_id_size = 0;
>> - d = sdev->vpd_pg83 + 4;
>> - while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
>> + d = rcu_dereference(sdev->vpd_pg83) + 4;
>> + while (d < rcu_dereference(sdev->vpd_pg83) + sdev->vpd_pg83_len) {
>
> Seem like this code would benefit from a local variable in favor of the
> repeated rcu_dereference() calls.
>
Yeah, possibly. After all, the variable isn't expected to change
under rcu_read_lock().
>> @@ -803,7 +803,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>
> I think this function could use a new name, e.g. scsi_read_vpd_pages?
>
>> @@ -563,8 +563,9 @@ static void ses_match_to_enclosure(struct enclosure_device *edev,
>> if (!sdev->vpd_pg83_len)
>> return;
>>
>> - desc = sdev->vpd_pg83 + 4;
>> - while (desc < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
>> + rcu_read_lock();
>> + desc = rcu_dereference(sdev->vpd_pg83) + 4;
>> + while (desc < rcu_dereference(sdev->vpd_pg83) + sdev->vpd_pg83_len) {
>
> A local variable or two would help here as well.
>
Okay.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
