[Could you trim a little when replying?]
On Tue, Sep 28, 2010 at 03:13:53PM +0200, Oliver Neukum wrote:
> Am Dienstag, 28. September 2010, 12:14:56 schrieb Matthew Wilcox:
> > +static void uas_do_work(struct work_struct *work)
> > +{
> > + struct uas_cmd_info *cmdinfo;
> > + struct list_head list;
> > +
> > + spin_lock_irq(&uas_work_lock);
> > + list_replace_init(&uas_work_list, &list);
> > + spin_unlock_irq(&uas_work_lock);
> > +
> > + list_for_each_entry(cmdinfo, &list, list) {
> > + struct scsi_pointer *scp = (void *)cmdinfo;
> > + struct scsi_cmnd *cmnd = container_of(scp,
> > + struct scsi_cmnd, SCp);
> > + uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_KERNEL);
>
> Deadlocky, use GFP_NOIO
ACK. I thought I'd fixed this already.
> > +static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
> > +{
> > + struct sense_iu *sense_iu = urb->transfer_buffer;
> > + struct scsi_device *sdev = cmnd->device;
> > +
> > + if (urb->actual_length > 16) {
> > + unsigned len = be16_to_cpup(&sense_iu->len);
> > + if (len + 16 != urb->actual_length) {
> > + int newlen = min(len + 16, urb->actual_length) - 16;
> > + if (newlen < 0)
> > + newlen = 0;
> > + sdev_printk(KERN_INFO, sdev, "%s: urb length %d "
> > + "disagrees with IU sense data length %d, "
> > + "using %d bytes of sense data\n", __func__,
> > + urb->actual_length, len, newlen);
> > + len = newlen;
> > + }
> > + memcpy(cmnd->sense_buffer, sense_iu->sense, len);
> > + }
> > +
> > + cmnd->result = sense_iu->status;
> > + if (sdev->current_cmnd)
> > + sdev->current_cmnd = NULL;
>
> the condition is absolutely senseless
That's not true. Avoiding dirtying a cacheline is important for
performance.
> > +static void uas_stat_cmplt(struct urb *urb)
> > +{
> > + struct iu *iu = urb->transfer_buffer;
> > + struct scsi_device *sdev = urb->context;
> > + struct uas_dev_info *devinfo = sdev->hostdata;
> > + struct scsi_cmnd *cmnd;
> > + u16 tag;
> > +
> > + if (urb->status) {
> > + dev_err(&urb->dev->dev, "URB BAD STATUS %d\n", urb->status);
> > + usb_free_urb(urb);
> > + return;
> > + }
> > +
> > + tag = be16_to_cpup(&iu->tag) - 1;
> > + if (sdev->current_cmnd)
> > + cmnd = sdev->current_cmnd;
> > + else
> > + cmnd = scsi_find_tag(sdev, tag);
> > + if (!cmnd)
> > + return;
>
> Where is the URB freed?
Good point. Will fix.
> > +static int uas_pre_reset(struct usb_interface *intf)
> > +{
> > +/* XXX: Need to return 1 if it's not our device in error handling */
> > + return 0;
> > +}
>
> You should probably make sure that no further commands are
> queued from this point on.
Yeah, there's a whole bunch of problems around error handling.
> > +static void uas_disconnect(struct usb_interface *intf)
> > +{
> > + struct usb_device *udev = interface_to_usbdev(intf);
> > + struct usb_host_endpoint *eps[3];
> > + struct Scsi_Host *shost = usb_get_intfdata(intf);
> > + struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
> > +
> > + scsi_remove_host(shost);
> > +
> > + eps[0] = usb_pipe_endpoint(udev, devinfo->status_pipe);
> > + eps[1] = usb_pipe_endpoint(udev, devinfo->data_in_pipe);
> > + eps[2] = usb_pipe_endpoint(udev, devinfo->data_out_pipe);
> > + usb_free_streams(intf, eps, 3, GFP_KERNEL);
>
> This implies that this can fail due to a lack of memory.
> Then what?
Uhh. I think I'll have to get Sarah to answer that question. We probably
end up resetting the endpoints instead of telling the endpoint to stop
doing streams.
Thanks for the review.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
