Re: [PATCH] [USB] UASP: USB Attached SCSI (UAS) protocol driver

[Luben Tuikov]

--- On Mon, 12/13/10, Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx> wrote:
> I think you may be missing some code review on the
> linux-usb mailing
> list.

There was never a review of uasp.c. Let's be clear on that. Because the way you've written the above sentence, someone may misread it to understand that your co-workers and Greg had written extensive review of uasp and had rejected it based on some tangible virtue. Greg's NAK came 4 minutes after I posted it and I mentioned that in my response (in this thread).

> Luben posted his original patch to the uas.c
> driver there, but he
> included behavior changes, code style modification, and
> variable
> renaming in one giant patch.

I wanted to rip the whole thing out. That patch changed a modest 86% of the code and I did mention that in the patch:
http://marc.info/?l=linux-usb&m=128901883420388&w=2

> It was very difficult to
> wade through,

That's why it's better to rip the whole thing out and put in something properly done.

> so
> I asked him to break the changes into a series of patches
> (perhaps 18,
> since he has 18 points?).

I'm not going to generate 18+ patches that changes 100% of that driver into a completely different and working driver.

First, I've no time for this. Second, it's much better, quicker and error-proof to present a whole _new_ piece (and show how it's done), moreover the fact that there are no UAS devices out there at the moment.

You and anyone else can certainly contribute to uasp.c. What's the point in converting the defunct uas.c to look like the working uasp.c (two different drivers)? The proper thing is to pull uas.c out and put uasp.c in (working and properly implemented (see my original editorial and patches to uas.c)).

> I did attempt to give a partial code review of Luben's
> original patches,
> and he has yet to respond to any of my points:
> 
> http://marc.info/?l=linux-kernel&m=129021816023869&w=2

Now that you asked, I just did respond to this. The reason I didn't respond to it before is because I didn't think there was any point in responding then, as I do now.  Back then I would've said: the whole thing should be ripped out and re-written. Now I'd say: refer to uasp.c on how it's done.  It's available on github.

> I also asked him to change a different patch for his
> driver, to move a
> work-around into a better place:
> 
> http://marc.info/?l=linux-kernel&m=129192831709856&w=2

And I responded to this in the next entry in the thread in the same link. The parameter still has value. Most likely it wouldn't have to be set (but it still has value in the very very rare case...).

> I have yet to see an updated patch set, only personal
> attacks.

A patch set for what? UAS? See uasp.c on how it's done. For the host that mis-calculates the number of streams it reports in HCCPARAMS? I still haven't had time to plug in a newer version of this HC. When I do and if it still misbehaves, I'll send you a dump of the PCI config space and you can decide what you want to do.

   Luben


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/