[PATCH] [USB] UAS: Rename urbs by pipe

[PATCH] [USB] UAS: Rename urbs by pipe

[Luben Tuikov]

The reason to do this is to accommodate sending
different IU types using the existing
infrastructure.

Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
---
 drivers/usb/storage/uas.c |   33 ++++++++++++++++-----------------
 1 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 17e1820..48dc2b8 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -97,8 +97,8 @@ struct uas_dev_info {
 };
 
 enum {
-	ALLOC_SENSE_URB		= (1 << 0),
-	SUBMIT_SENSE_URB	= (1 << 1),
+	ALLOC_STATUS_URB	= (1 << 0),
+	SUBMIT_STATUS_URB	= (1 << 1),
 	ALLOC_DATA_IN_URB	= (1 << 2),
 	SUBMIT_DATA_IN_URB	= (1 << 3),
 	ALLOC_DATA_OUT_URB	= (1 << 4),
@@ -112,13 +112,12 @@ struct uas_cmd_info {
 	unsigned int state;
 	unsigned int stream;
 	struct urb *cmd_urb;
-	struct urb *sense_urb;
+	struct urb *status_urb;
 	struct urb *data_in_urb;
 	struct urb *data_out_urb;
 	struct list_head list;
 };
 
-/* I hate forward declarations, but I actually have a loop */
 static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 				struct uas_dev_info *devinfo, gfp_t gfp);
 
@@ -204,7 +203,7 @@ static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 	int err;
 
-	cmdinfo->state = direction | SUBMIT_SENSE_URB;
+	cmdinfo->state = direction | SUBMIT_STATUS_URB;
 	err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
 	if (err) {
 		spin_lock(&uas_work_lock);
@@ -360,21 +359,21 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 {
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 
-	if (cmdinfo->state & ALLOC_SENSE_URB) {
-		cmdinfo->sense_urb = uas_alloc_sense_urb(devinfo, gfp, cmnd,
-							cmdinfo->stream);
-		if (!cmdinfo->sense_urb)
+	if (cmdinfo->state & ALLOC_STATUS_URB) {
+		cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp, cmnd,
+							  cmdinfo->stream);
+		if (!cmdinfo->status_urb)
 			return SCSI_MLQUEUE_DEVICE_BUSY;
-		cmdinfo->state &= ~ALLOC_SENSE_URB;
+		cmdinfo->state &= ~ALLOC_STATUS_URB;
 	}
 
-	if (cmdinfo->state & SUBMIT_SENSE_URB) {
-		if (usb_submit_urb(cmdinfo->sense_urb, gfp)) {
+	if (cmdinfo->state & SUBMIT_STATUS_URB) {
+		if (usb_submit_urb(cmdinfo->status_urb, gfp)) {
 			scmd_printk(KERN_INFO, cmnd,
 					"sense urb submission failure\n");
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
-		cmdinfo->state &= ~SUBMIT_SENSE_URB;
+		cmdinfo->state &= ~SUBMIT_STATUS_URB;
 	}
 
 	if (cmdinfo->state & ALLOC_DATA_IN_URB) {
@@ -443,7 +442,7 @@ static int uas_queuecommand(struct scsi_cmnd *cmnd,
 
 	BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));
 
-	if (!cmdinfo->sense_urb && sdev->current_cmnd)
+	if (!cmdinfo->status_urb && sdev->current_cmnd)
 		return SCSI_MLQUEUE_DEVICE_BUSY;
 
 	if (blk_rq_tagged(cmnd->request)) {
@@ -455,7 +454,7 @@ static int uas_queuecommand(struct scsi_cmnd *cmnd,
 
 	cmnd->scsi_done = done;
 
-	cmdinfo->state = ALLOC_SENSE_URB | SUBMIT_SENSE_URB |
+	cmdinfo->state = ALLOC_STATUS_URB | SUBMIT_STATUS_URB |
 			ALLOC_CMD_URB | SUBMIT_CMD_URB;
 
 	switch (cmnd->sc_data_direction) {
@@ -478,8 +477,8 @@ static int uas_queuecommand(struct scsi_cmnd *cmnd,
 	err = uas_submit_urbs(cmnd, devinfo, GFP_ATOMIC);
 	if (err) {
 		/* If we did nothing, give up now */
-		if (cmdinfo->state & SUBMIT_SENSE_URB) {
-			usb_free_urb(cmdinfo->sense_urb);
+		if (cmdinfo->state & SUBMIT_STATUS_URB) {
+			usb_free_urb(cmdinfo->status_urb);
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
 		spin_lock(&uas_work_lock);
-- 
1.7.0.1

Re: [PATCH] [USB] UAS: Rename urbs by pipe

[Greg KH]

On Thu, Oct 28, 2010 at 01:05:14PM -0700, Luben Tuikov wrote:
> The reason to do this is to accommodate sending
> different IU types using the existing
> infrastructure.

I'm sorry, but can you explain this a bit more?  I really don't
understand why this change is necessary.

thanks,

greg k-h

Re: [PATCH] [USB] UAS: Rename urbs by pipe

[Luben Tuikov]

--- On Thu, 10/28/10, Greg KH <greg@kroah.com> wrote:
> I'm sorry, but can you explain this a bit more?  I
> really don't
> understand why this change is necessary.

It isn't necessary (like water or food, etc). It reflects much closer what
really is happening under the hood. Having a software abstraction
(implementation) representing the hw or the physical function much closer,
allows for much more versatile manipulation.

For example, if you would like to implement sending TMFs and receiving
their Responses, you'd do that on the Command and Status pipes. So you can
allocate an URB at the "cmd_urb" (meaning "command pipe urb") and on the
"status_urb" (meaning "status pipe urb"). Before this patch this was called
"sense_urb" which reflected what the type of IU but not the pipe, all the
while it was being allocated for the Status pipe. A good infrastructure
is a reusable infrastructure. This patch properly renames some macro
definitions and variables in order to allow for some of the infrastructure
of the driver to be reused in order to send other IUs.

Re: [PATCH] [USB] UAS: Rename urbs by pipe

[Matthew Wilcox]

On Thu, Oct 28, 2010 at 01:05:14PM -0700, Luben Tuikov wrote:
> The reason to do this is to accommodate sending
> different IU types using the existing
> infrastructure.

Applied, with a modified changelog entry ...

>  	struct urb *data_out_urb;
>  	struct list_head list;
>  };
>  
> -/* I hate forward declarations, but I actually have a loop */
>  static int uas_submit_urbs(struct scsi_cmnd *cmnd,
>  				struct uas_dev_info *devinfo, gfp_t gfp);
>  

Except for this spurious chunk.

Re: [PATCH] [USB] UAS: Rename urbs by pipe

[Luben Tuikov]

--- On Mon, 11/8/10, Matthew Wilcox <willy@linux.intel.com> wrote:
> -0700, Luben Tuikov wrote:
> > The reason to do this is to accommodate sending
> > different IU types using the existing
> > infrastructure.
> 
> Applied, with a modified changelog entry ...

Again, where is it applied and what is the (new) modified changelog entry? Why would you leave this detail out in your response to the patch?

> 
> >      struct urb *data_out_urb;
> >      struct list_head list;
> >  };
> >  
> > -/* I hate forward declarations, but I actually have a
> loop */
> >  static int uas_submit_urbs(struct scsi_cmnd
> *cmnd,
> >         
>         struct uas_dev_info
> *devinfo, gfp_t gfp);
> >  
> 
> Except for this spurious chunk.

Sorry, please help us understand: You'll LEAVE a self-gratifying, narcissistic and pompous statement like "I hate forward-declarations"? Why would you use such a strong word? Why would you "hate"? How about: "I don't like" or "I know lk style discourages", etc. Why would you add /one more/ "hate" word into the kernel?

I think everyone sees why this forward-decl is needed.

I wonder if the relation of the number of the "hate" word in the linux kernel versus time correlates to something, anything? Now that's easier to track and analyze in time using git.

Compared to people losing their homes and jobs nowadays, I'll take a forward declaration any time of day and night.

Bottom line: please consider the patch as is.

Re: [PATCH] [USB] UAS: Rename urbs by pipe

[Greg KH]

On Thu, Oct 28, 2010 at 01:05:14PM -0700, Luben Tuikov wrote:
> The reason to do this is to accommodate sending
> different IU types using the existing
> infrastructure.
> 
> Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>

Matthew, it's been over a month, please comment on these UAS patches, or
I guess I'll just apply them...

thanks,

greg k-h

Re: [PATCH] [USB] UAS: Rename urbs by pipe

[Matthew Wilcox]

On Tue, Nov 30, 2010 at 05:07:15PM -0800, Greg KH wrote:
> On Thu, Oct 28, 2010 at 01:05:14PM -0700, Luben Tuikov wrote:
> > The reason to do this is to accommodate sending
> > different IU types using the existing
> > infrastructure.
> > 
> > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> 
> Matthew, it's been over a month, please comment on these UAS patches, or
> I guess I'll just apply them...

Hi Greg,

These patches are NAKed in their current form.  Luben does not care for how
I have modified them, so I need to rework them to take his name off them.
I'll have a git tree ready for you after the weekend.

Re: [PATCH] [USB] UAS: Rename urbs by pipe

[Luben Tuikov]

--- On Thu, 12/2/10, Matthew Wilcox <willy@linux.intel.com> wrote:
> > On Thu, Oct 28, 2010 at 01:05:14PM -0700, Luben Tuikov
> wrote:
> > > The reason to do this is to accommodate sending
> > > different IU types using the existing
> > > infrastructure.
> > > 
> > > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> > 
> > Matthew, it's been over a month, please comment on
> these UAS patches, or
> > I guess I'll just apply them...
> 
> Hi Greg,
> 
> These patches are NAKed in their current form.  Luben
> does not care for how
> I have modified them, so I need to rework them to take his
> name off them.
> I'll have a git tree ready for you after the weekend.

Are you serious? My commit message is quoted above. This email thread is available at http://marc.info/?t=128829652800022&r=1&w=2
as are my other patches to your code, where you stated you've committed them all but changed the commit logs (as typed by me). When asked to provide your changes to the commit logs in this very thread, you didn't. *Please don't twist the facts as you did my commit messages to suit your own needs.* 

Re: [PATCH] [USB] UAS: Rename urbs by pipe

[Greg KH]

On Thu, Dec 02, 2010 at 02:26:42PM -0800, Luben Tuikov wrote:
> --- On Thu, 12/2/10, Matthew Wilcox <willy@linux.intel.com> wrote:
> > > On Thu, Oct 28, 2010 at 01:05:14PM -0700, Luben Tuikov
> > wrote:
> > > > The reason to do this is to accommodate sending
> > > > different IU types using the existing
> > > > infrastructure.
> > > > 
> > > > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> > > 
> > > Matthew, it's been over a month, please comment on
> > these UAS patches, or
> > > I guess I'll just apply them...
> > 
> > Hi Greg,
> > 
> > These patches are NAKed in their current form.

I never saw those NAKs, please always copy the original people who were
copied on the patch when you do that.  The list never saw them either.

> >  Luben
> > does not care for how
> > I have modified them, so I need to rework them to take his
> > name off them.

Where did you rework them?  When were those patches posted?

> > I'll have a git tree ready for you after the weekend.

No, I want patches in email please.

> Are you serious? My commit message is quoted above. This email thread
> is available at http://marc.info/?t=128829652800022&r=1&w=2 as are my
> other patches to your code, where you stated you've committed them all
> but changed the commit logs (as typed by me). When asked to provide
> your changes to the commit logs in this very thread, you didn't.
> *Please don't twist the facts as you did my commit messages to suit
> your own needs.* 

I agree, this seems very odd.

Mathew, please be more public about what is going on here.

thanks,

greg k-h

Re: [PATCH] [USB] UAS: Rename urbs by pipe

[Luben Tuikov]

Patch retracted for this still-born driver. Instead please use superior uasp.c which was recently submitted.


--- On Thu, 10/28/10, Luben Tuikov <ltuikov@yahoo.com> wrote:

> From: Luben Tuikov <ltuikov@yahoo.com>
> Subject: [PATCH] [USB] UAS: Rename urbs by pipe
> To: "Greg KH" <greg@kroah.com>, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, "Matthew Wilcox" <willy@linux.intel.com>
> Date: Thursday, October 28, 2010, 1:05 PM
> The reason to do this is to
> accommodate sending
> different IU types using the existing
> infrastructure.
> 
> Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> ---
>  drivers/usb/storage/uas.c |   33
> ++++++++++++++++-----------------
>  1 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c
> b/drivers/usb/storage/uas.c
> index 17e1820..48dc2b8 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -97,8 +97,8 @@ struct uas_dev_info {
>  };
>  
>  enum {
> -    ALLOC_SENSE_URB   
>     = (1 << 0),
> -    SUBMIT_SENSE_URB    = (1
> << 1),
> +    ALLOC_STATUS_URB    = (1
> << 0),
> +    SUBMIT_STATUS_URB    =
> (1 << 1),
>      ALLOC_DATA_IN_URB    =
> (1 << 2),
>      SUBMIT_DATA_IN_URB    =
> (1 << 3),
>      ALLOC_DATA_OUT_URB    =
> (1 << 4),
> @@ -112,13 +112,12 @@ struct uas_cmd_info {
>      unsigned int state;
>      unsigned int stream;
>      struct urb *cmd_urb;
> -    struct urb *sense_urb;
> +    struct urb *status_urb;
>      struct urb *data_in_urb;
>      struct urb *data_out_urb;
>      struct list_head list;
>  };
>  
> -/* I hate forward declarations, but I actually have a loop
> */
>  static int uas_submit_urbs(struct scsi_cmnd *cmnd,
>             
>     struct uas_dev_info *devinfo, gfp_t
> gfp);
>  
> @@ -204,7 +203,7 @@ static void uas_xfer_data(struct urb
> *urb, struct scsi_cmnd *cmnd,
>      struct uas_cmd_info *cmdinfo = (void
> *)&cmnd->SCp;
>      int err;
>  
> -    cmdinfo->state = direction |
> SUBMIT_SENSE_URB;
> +    cmdinfo->state = direction |
> SUBMIT_STATUS_URB;
>      err = uas_submit_urbs(cmnd,
> cmnd->device->hostdata, GFP_ATOMIC);
>      if (err) {
>         
> spin_lock(&uas_work_lock);
> @@ -360,21 +359,21 @@ static int uas_submit_urbs(struct
> scsi_cmnd *cmnd,
>  {
>      struct uas_cmd_info *cmdinfo = (void
> *)&cmnd->SCp;
>  
> -    if (cmdinfo->state &
> ALLOC_SENSE_URB) {
> -       
> cmdinfo->sense_urb = uas_alloc_sense_urb(devinfo, gfp,
> cmnd,
> -           
>            
>     cmdinfo->stream);
> -        if
> (!cmdinfo->sense_urb)
> +    if (cmdinfo->state &
> ALLOC_STATUS_URB) {
> +       
> cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp,
> cmnd,
> +           
>            
>       cmdinfo->stream);
> +        if
> (!cmdinfo->status_urb)
>             
> return SCSI_MLQUEUE_DEVICE_BUSY;
> -        cmdinfo->state
> &= ~ALLOC_SENSE_URB;
> +        cmdinfo->state
> &= ~ALLOC_STATUS_URB;
>      }
>  
> -    if (cmdinfo->state &
> SUBMIT_SENSE_URB) {
> -        if
> (usb_submit_urb(cmdinfo->sense_urb, gfp)) {
> +    if (cmdinfo->state &
> SUBMIT_STATUS_URB) {
> +        if
> (usb_submit_urb(cmdinfo->status_urb, gfp)) {
>             
> scmd_printk(KERN_INFO, cmnd,
>             
>         "sense urb submission
> failure\n");
>             
> return SCSI_MLQUEUE_DEVICE_BUSY;
>          }
> -        cmdinfo->state
> &= ~SUBMIT_SENSE_URB;
> +        cmdinfo->state
> &= ~SUBMIT_STATUS_URB;
>      }
>  
>      if (cmdinfo->state &
> ALLOC_DATA_IN_URB) {
> @@ -443,7 +442,7 @@ static int uas_queuecommand(struct
> scsi_cmnd *cmnd,
>  
>      BUILD_BUG_ON(sizeof(struct
> uas_cmd_info) > sizeof(struct scsi_pointer));
>  
> -    if (!cmdinfo->sense_urb &&
> sdev->current_cmnd)
> +    if (!cmdinfo->status_urb &&
> sdev->current_cmnd)
>          return
> SCSI_MLQUEUE_DEVICE_BUSY;
>  
>      if (blk_rq_tagged(cmnd->request)) {
> @@ -455,7 +454,7 @@ static int uas_queuecommand(struct
> scsi_cmnd *cmnd,
>  
>      cmnd->scsi_done = done;
>  
> -    cmdinfo->state = ALLOC_SENSE_URB |
> SUBMIT_SENSE_URB |
> +    cmdinfo->state = ALLOC_STATUS_URB |
> SUBMIT_STATUS_URB |
>             
> ALLOC_CMD_URB | SUBMIT_CMD_URB;
>  
>      switch (cmnd->sc_data_direction) {
> @@ -478,8 +477,8 @@ static int uas_queuecommand(struct
> scsi_cmnd *cmnd,
>      err = uas_submit_urbs(cmnd, devinfo,
> GFP_ATOMIC);
>      if (err) {
>          /* If we did
> nothing, give up now */
> -        if
> (cmdinfo->state & SUBMIT_SENSE_URB) {
> -           
> usb_free_urb(cmdinfo->sense_urb);
> +        if
> (cmdinfo->state & SUBMIT_STATUS_URB) {
> +           
> usb_free_urb(cmdinfo->status_urb);
>             
> return SCSI_MLQUEUE_DEVICE_BUSY;
>          }
>         
> spin_lock(&uas_work_lock);
> -- 
> 1.7.0.1
> 
> 

Re: [PATCH] [USB] UAS: Rename urbs by pipe

[Luben Tuikov]

--- On Mon, 11/8/10, Luben Tuikov <ltuikov@yahoo.com> wrote:
> --- On Mon, 11/8/10, Matthew Wilcox
> <willy@linux.intel.com>
> wrote:
> > -0700, Luben Tuikov wrote:
> > > The reason to do this is to accommodate sending
> > > different IU types using the existing
> > > infrastructure.
> > 
> > Applied, with a modified changelog entry ...
> 
> Again, where is it applied and what is the (new) modified
> changelog entry? Why would you leave this detail out in your
> response to the patch?

Please have some professional integrity and EITHER a) ask me to
resubmit the patch with a new change log (what would you like?) or b) discuss it here publicly.

See my previous post on your other modification of the change log
to my other patch against your code.

> 
> > 
> > >      struct urb *data_out_urb;
> > >      struct list_head list;
> > >  };
> > >  
> > > -/* I hate forward declarations, but I actually
> have a
> > loop */
> > >  static int uas_submit_urbs(struct scsi_cmnd
> > *cmnd,
> > >         
> >         struct uas_dev_info
> > *devinfo, gfp_t gfp);
> > >  
> > 
> > Except for this spurious chunk.
> 
> Sorry, please help us understand: You'll LEAVE a
> self-gratifying, narcissistic and pompous statement like "I
> hate forward-declarations"? Why would you use such a strong
> word? Why would you "hate"? How about: "I don't like" or "I
> know lk style discourages", etc. Why would you add /one
> more/ "hate" word into the kernel?
> 
> I think everyone sees why this forward-decl is needed.
> 
> I wonder if the relation of the number of the "hate" word
> in the linux kernel versus time correlates to something,
> anything? Now that's easier to track and analyze in time
> using git.
> 
> Compared to people losing their homes and jobs nowadays,
> I'll take a forward declaration any time of day and night.
> 
> Bottom line: please consider the patch as is.
> 
>