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

[Juergen Gross <jgross@xxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel