[dm-devel] Re: [PATCH 1/7] scsi_dh: Implement generic device table handling

[Hannes Reinecke]

Chandra Seetharaman wrote:
> On Wed, 2008-05-14 at 16:43 +0200, Hannes Reinecke wrote:
> <snip>
> 
>> +
>> +struct scsi_dh_devinfo_list {
>> +	struct list_head node;
>> +	char vendor[9];
>> +	char model[17];
>> +	struct scsi_device_handler *handler;
>> +};
> 
> I do not see why we should be duplicating the information that
> is already present in scsi_dh and is available here thru scsi_dh_list.
> Instead of walking thru the scsi_dh_dev_list we could walk thru
> scsi_dh_list and get the same result.
>
Indeed we can. I originally did this as it would allow us to dynamically
add and remove entries from that list, which isn't possible with the
static arrays provided by the individual device handler.
But seeing that we can now manually attach any device I guess we can
remove it.
 
> On the other hand, if we could create a cache of the list of devices
> (vendor-product-scsi_dh triplet) that has been attached, and walk
> thru that list first before others during attach, it would help
> (as we would guess there to be multiple of the same device).
>  
Yes, but we can achieve that very same thing by walking the scsi_dh_list.
I'll fix it up.

>> +
>> +int scsi_dh_handler_attach(struct scsi_device *sdev,
>> +			   struct scsi_device_handler *scsi_dh);
>> +int scsi_dh_handler_detach(struct scsi_device *sdev);
> 
> Why is the prototype needed ?
> 
> Can this be static ?
> 
Probably a left over from earlier attempts. Will be removing them.

>>  static struct scsi_device_handler *get_device_handler(const char *name)
>>  {
>> @@ -33,7 +45,7 @@ static struct scsi_device_handler *get_device_handler(const char *name)
>>
> 
> <snip>
> 
>> +/*
>> + * scsi_dh_handler_attach - Attach a device handler to a device
>> + * @sdev - SCSI device the device handler should attach to
>> + * @scsi_dh - The device handler to attach
>> + */
>> +int scsi_dh_handler_attach(struct scsi_device *sdev,
>> +			   struct scsi_device_handler *scsi_dh)
>> +{
>> +	int err = -EBUSY;
>> +
>> +	if (sdev->scsi_dh_data)
>> +		return err;
> 
> One of the later patch check for scsi_dh to be same as
> sdev->scsi_dh_data->scsi_dh. We can check for that here.
> 
> One more thing: there are multiple places (in this file and
> in the hardware handlers) the same check appear. Can we just
> do it only here and remove all the others ?
>
Yes. Will do.
 
>> +
>> +	err = scsi_dh->attach(sdev);
> 
> This assumes attach is available, but there is no assertion in
> scsi_register_device_handler().  Same is the case with detach(). Either
> assert or check for the function availability here.
Ok.

>> +
>> +	return err;
>> +}
>> +
>> +/*
>> + * scsi_dh_handler_detach - Detach a device handler to a device
>> + * @sdev - SCSI device the device handler should be detached from
>> + */
>> +int scsi_dh_handler_detach(struct scsi_device *sdev)
>> +{
>> +	struct scsi_device_handler *scsi_dh;
> 
> can avoid the variable and call detach directly ?
>> +
>> +	if (!sdev->scsi_dh_data)
>> +		return -ENODEV;
> 
> Can't we just return 0 ?
> 
Well, seeing that the actual detach callback is
of type 'void', we should be using the same type here.

>> +
>> +	scsi_dh = sdev->scsi_dh_data->scsi_dh;
>> +
>> +	scsi_dh->detach(sdev);
>> +
>> +	return 0;
>> +}
> 
> <snip>
> 
>>  	list_add(&scsi_dh->list, &scsi_dh_list);
>>  	spin_unlock(&list_lock);
>> +	bus_for_each_dev(&scsi_bus_type, NULL, scsi_dh, scsi_dh_notifier_add);
>> +	printk(KERN_INFO "%s: registered\n", scsi_dh->name);
> 
> We can be more descriptive. Like "Hardware handler %s registered" or some such.
> 
Ok.

>> +	ret = SCSI_DH_OK;
>>
>>  done:
>>  	return ret;
> <snip>
> 
>> +	}
>>  	list_del(&scsi_dh->list);
>>  	spin_unlock(&list_lock);
>> +	printk(KERN_INFO "%s: unregistered\n", scsi_dh->name);
> 
> Same here.
> 
OK.

Thanks for the review. I'll post an update soon.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)