Re: [Xen-devel] [PATCH V4 2/4] Introduce xen-scsifront module

[Juergen Gross <jgross@suse.com>]

On 08/11/2014 11:54 AM, Christoph Hellwig wrote:
>> +	BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
>> +
>> +	if (sc->cmd_len)
>
> I can't see how you can get a zero cmd_len here.

Ahh, thanks for spotting this. In a previous version it could be zero
in case of reset.

>> +static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
>
> Please add a comment explaining your unusual EH strategy here.

What do you mean with "unusual"? You mean transferring the EH action to
Dom0?

>
>> +static void scsifront_free(struct vscsifrnt_info *info)
>> +{
>> +	if (info->host && info->host_active) {
>> +		/* Scsi_host not yet removed */
>> +		scsi_remove_host(info->host);
>> +		info->host_active = 0;
>> +	}
>> +
>> +	if (info->ring_ref != GRANT_INVALID_REF) {
>> +		gnttab_end_foreign_access(info->ring_ref, 0,
>> +					  (unsigned long)info->ring.sring);
>> +		info->ring_ref = GRANT_INVALID_REF;
>> +		info->ring.sring = NULL;
>> +	}
>> +
>> +	if (info->irq)
>> +		unbind_from_irqhandler(info->irq, info);
>> +	info->irq = 0;
>> +	info->evtchn = 0;
>> +
>> +	if (info->host)
>> +		scsi_host_put(info->host);
>> +}
>
> I don't think most of the ifs should be here, just use proper symmetric
> goto unwinding in the initialization error path instead.
>
> The way this function can be called from different levels of the
> callstack on init failure is very confusing.

Okay, I'll look into making it easier to understand.

>
>> +		switch (op) {
>> +		case VSCSIFRONT_OP_ADD_LUN:
>> +			if (device_state == XenbusStateInitialised) {
>> +				sdev = __scsi_add_device(info->host, chn, tgt,
>> +							lun, NULL);
>> +				err = (IS_ERR(sdev) || !sdev->hostdata);
>> +				if (!IS_ERR(sdev)) {
>> +					sdev->hostdata = NULL;
>> +					scsi_device_put(sdev);
>> +				}
>
> Given that you put the device immediatly you should be using
> scsi_add_device instead of __scsi_add_device.  Also all the messing
> with ->hostdata from ->slave_alloc looks wrong.  For one thing every
> setup done ->slave_alloc should be paired with teardown in
> ->slave_destroy.  Second I don't see any need for that.

The problem is I have to take different actions depending on the device
being new or not.

>
>> +				} else {
>> +					xenbus_printf(XBT_NIL, dev->nodename,
>> +						      state_str, "%d",
>> +						      XenbusStateConnected);
>> +				}
>
> Just print this message in ->slave_configure.

This is calling for problems, I think. xenbus_printf() is not just a
printing function, but it changes an entry in the xenstore. And this
requires locking, switching threads, ...

I doubt doing this while holding SCSI-internal locks is a good idea.


Juergen