[U-Boot] USB mass storage gadget patch

[U-Boot] USB mass storage gadget patch

[victor]

Lukasz,

I am looking at the usb mass storage gadget patch from you at:
http://patchwork.ozlabs.org/patch/104516/

I am trying to use this patch in the u-boot. Some questions:
1) Is this patch in U-boot git release?
2) Now I can use the patch and process till SCSI MODE_SELECT_6 command, and
then problem is encountered. Basically the SCSI MODE_SELECT_6 is sent to EP1
Out, then data is sent to EP1 out, and the fsg_main_thread only sees the
data. I change the code to force the CSW.

The changes are:
1) fix the buf pointer in scsi inquiry
2) fix the buf pointer in scsi read format capacities
3) fix the buf pointer in CSW sending
4) fix attention condition checking in scsi command check
5) fix the context pointer before queue a request to read bulk only CBW
6) change bulk out endpoint from endpoint 2 to endpoint 1 to accommodate
gen2 usb hardware limitation
7) in pad_with_zeros(), comment out the bh->inreq->length = nsend
8) in received_cbw(), force to send CSW if CBW is invalid. Otherwise, the
IRQ routine keeps on reading the same data.

I attach the file for your reference. Please kindly advise me. Thanks.

Thanks,
victor

================================================================================================
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information and may be protected by legal privilege. If you are not the intended addressee (or authorized to receive for the addressee). be aware that any disclosure, copying, distribution or use of this e-mail or any attachment is prohibited. If you have received this e-mail in error, please notify us immediately by returning it to the sender and delete this copy from your system. Thank you for your cooperation.
KeyASIC Inc.
================================================================================================
-------------- next part --------------
A non-text attachment was scrubbed...
Name: file_storage.c
Type: application/octet-stream
Size: 106931 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121102/f6d8c100/attachment.obj>

[U-Boot] USB mass storage gadget patch

[Lukasz Majewski]

Hi victor,

> Lukasz,
> 
> I am looking at the usb mass storage gadget patch from you at:

USB mass storage gadget reference:
[*]
http://patchwork.ozlabs.org/patch/104516/
> 
> I am trying to use this patch in the u-boot. Some questions:
> 1) Is this patch in U-boot git release?

Unfortunately not. It has been posted, but so far wasn't accepted to
mainline u-boot.

The reason is that in meantime I've added composite gadget support for
the u-boot USB stack. DFU "function" uses this composite gadget
framework.
 
UMS (ver. [*]) shall also be modified to work with the composite
gadget framework. Posted patch [*] works as a standalone USB
gadget.

> 2) Now I can use the patch and process till SCSI MODE_SELECT_6
> command, and then problem is encountered. Basically the SCSI
> MODE_SELECT_6 is sent to EP1 Out, then data is sent to EP1 out, and
> the fsg_main_thread only sees the data. I change the code to force
> the CSW.

Can you explain further problems which you encounter?

I'm using this UMS version internally (the version which you pointed
out with the link) for debugging our targets.

I'm using Debian Linux as a host with standard tools like dd and
parted/fdisk. I don't have any problems with it (even with debug
messages enabled).


[*] implementation of UMS has one (bit) flaw:
It doesn't provide "caching" for read/write operations. Therefore
sending bigger files is very time consuming.

This and support for composite gadget infrastructure shall be developed
for v3 of UMS patch set.

> 
> The changes are:
> 1) fix the buf pointer in scsi inquiry
> 2) fix the buf pointer in scsi read format capacities
> 3) fix the buf pointer in CSW sending
> 4) fix attention condition checking in scsi command check
> 5) fix the context pointer before queue a request to read bulk only
> CBW 6) change bulk out endpoint from endpoint 2 to endpoint 1 to
> accommodate gen2 usb hardware limitation
> 7) in pad_with_zeros(), comment out the bh->inreq->length = nsend
> 8) in received_cbw(), force to send CSW if CBW is invalid. Otherwise,
> the IRQ routine keeps on reading the same data.
> 
> I attach the file for your reference. Please kindly advise me. Thanks.

Thanks for your fixes, but as I said previously, could you put some more
shed on the problem? 


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] USB mass storage gadget patch

[victor]

Lukasz,

>> 2) Now I can use the patch and process till SCSI MODE_SELECT_6 
>> command, and then problem is encountered. Basically the SCSI
>> MODE_SELECT_6 is sent to EP1 Out, then data is sent to EP1 out, and 
>> the fsg_main_thread only sees the data. I change the code to force the 
>> CSW.
>
>Can you explain further problems which you encounter?
>
>I'm using this UMS version internally (the version w		hich you
pointed out with the link) for debugging our targets.
>
>I'm using Debian Linux as a host with standard tools like dd and
parted/fdisk. I don't have any problems with it (even with debug messages
enabled).
>
>

In cmd_usb_mass_storage.c, do_usb_mass_storage(), the while(1) loop, the
usb_gadget_handle_interrupts() will handle the interrupts that is triggered,
and received data from EP1 out, and bh->state is set to BUF_STATE_FULL in
bulk_out_complete(). And then fsg_main_thread() in file_storage.c is called.
And then get_next_command() is called. And then start_transfer() is called.
In that function, the bh->state will be set to BUF_STATE_BUSY. Because of
this change, for two consecutive data from EP1 out, one data is missed.

I can send the full source code to you if it is helpful. Thanks.

Regards,
Victor


================================================================================================
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information and may be protected by legal privilege. If you are not the intended addressee (or authorized to receive for the addressee). be aware that any disclosure, copying, distribution or use of this e-mail or any attachment is prohibited. If you have received this e-mail in error, please notify us immediately by returning it to the sender and delete this copy from your system. Thank you for your cooperation.
KeyASIC Inc.
================================================================================================

[U-Boot] USB mass storage gadget patch

[victor]

Lukasz,

>>> 2) Now I can use the patch and process till SCSI MODE_SELECT_6 
>>> command, and then problem is encountered. Basically the SCSI
>>> MODE_SELECT_6 is sent to EP1 Out, then data is sent to EP1 out, and 
>>> the fsg_main_thread only sees the data. I change the code to force 
>>> the CSW.
>>
>>Can you explain further problems which you encounter?
>>
>>I'm using this UMS version internally (the version which you pointed out
with the link) for debugging our targets.
>>
>>I'm using Debian Linux as a host with standard tools like dd and
parted/fdisk. I don't have any problems with it (even with debug messages
enabled).
>>
>>
>
>In cmd_usb_mass_storage.c, do_usb_mass_storage(), the while(1) loop, the
usb_gadget_handle_interrupts() will handle the interrupts that is triggered,
and received data
> from EP1 out, and bh-state >is set to BUF_STATE_FULL in
bulk_out_complete(). And then fsg_main_thread() in file_storage.c is called.
And then get_next_command() is called. 
>And then start_transfer() is called. In that function, the bh->state will
be set to BUF_STATE_BUSY. Because of this change, for two consecutive data
from EP1 out, one data is missed.

>I can send the full source code to you if it is helpful. Thanks.

On further research into this, when receiving the SCSI_MODE_SELECT_6
command, in bulk_out_complete(), the bh pointer is 0x1b872e4

====== debug statement from console ======
0x43425355 0x507bc10 0x18 0x15060000 0x18000010 0x0 0x0 
bulk_out_complete 0x1b872e4

Then, in get_next_command(), before calling start_transfer(), the bh pointer
becomes 0x1b87304. 

This changes in bh pointer can cause data not to be processed by the usb
mass storage gadget code. 

Regards,
Victor


================================================================================================
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information and may be protected by legal privilege. If you are not the intended addressee (or authorized to receive for the addressee). be aware that any disclosure, copying, distribution or use of this e-mail or any attachment is prohibited. If you have received this e-mail in error, please notify us immediately by returning it to the sender and delete this copy from your system. Thank you for your cooperation.
KeyASIC Inc.
================================================================================================

[U-Boot] USB mass storage gadget patch

[victor]

Hi,

>On further research into this, when receiving the SCSI_MODE_SELECT_6
command, in bulk_out_complete(), the bh pointer is 0x1b872e4
>
>====== debug statement from console ======
>0x43425355 0x507bc10 0x18 0x15060000 0x18000010 0x0 0x0 bulk_out_complete
0x1b872e4
>
>Then, in get_next_command(), before calling start_transfer(), the bh
pointer becomes 0x1b87304. 
>
>This changes in bh pointer can cause data not to be processed by the usb
mass storage gadget code. 

Is there any feedback on the buffer management problem in the U-boot usb
mass storage gadget patch? Thanks.

Regards,
Victor


================================================================================================
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information and may be protected by legal privilege. If you are not the intended addressee (or authorized to receive for the addressee). be aware that any disclosure, copying, distribution or use of this e-mail or any attachment is prohibited. If you have received this e-mail in error, please notify us immediately by returning it to the sender and delete this copy from your system. Thank you for your cooperation.
KeyASIC Inc.
================================================================================================

[U-Boot] USB mass storage gadget patch

[Lukasz Majewski]

Hi victor,

Regarding UMS support:

The last version posted at u-boot ML can be found at [**]:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/113004/match=ums



I've go through your fixes for file_storage.c. 


Please find appended diff file [*] for changes which I was able to
apply on the original file_storage.c file (taken from [**]).

I've attached output from Host dmesg and target's console (output.txt)

It seems that on my setup your patches are causing errors and UMS is
not working (e.g. partitions aren't exported).

I don't have enough time now to delve to your solution, but I'm
skeptical about hard setting EPs (like below).

+if (fsg_fs_bulk_out_desc.bEndpointAddress != 0x1)
+	fsg_fs_bulk_out_desc.bEndpointAddress = 0x1;
+

If possible, please align to patch set [**], so we can have the same
setup.

However, I will try to find some time to help you with the problem
(that's why I've asked for description of the problem, which you are
trying to fix).

-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-UMS-CHanges-made-by-Victor-applied-to-UMS_v2-file-ap.patch
Type: text/x-patch
Size: 83556 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121106/6d847b5e/attachment.bin>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: output.txt
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121106/6d847b5e/attachment.txt>

[U-Boot] USB mass storage gadget patch

[victor]

Lukasz,

> Hi victor,
> 
> Regarding UMS support:
> 
> The last version posted at u-boot ML can be found at [**]:
> http://thread.gmane.org/gmane.comp.boot-loaders.u-
> boot/113004/match=ums
> 
> 
> +if (fsg_fs_bulk_out_desc.bEndpointAddress != 0x1)
> +	fsg_fs_bulk_out_desc.bEndpointAddress = 0x1;
> +
> 
> If possible, please align to patch set [**], so we can have the same
setup.
> 
> However, I will try to find some time to help you with the problem (that's
> why I've asked for description of the problem, which you are trying to
fix).
> 

I hardcode bulk out endpoint to EP1 because the hardware only got one
endpoint. So the code has to use EP1 for both bulk in and bulk out data
transfer. Will this break the usb mass storage gadget code?

I will get the file_storage.c  from [**].

The short description of the problem:
The U-boot usb mass storage gadget driver has problem with buffer
management, sometimes the buffer returned by IRQ routine and the
get_next_command function are different. It causes problem in processing
SCSI_MODE_SELECT_6 command. 

The detailed description of the problem:
When receiving the SCSI_MODE_SELECT_6 command, in bulk_out_complete(), the
bh pointer is 0x1b872e4. Then, in get_next_command(), before calling
start_transfer(), the bh pointer becomes 0x1b87304. This changes in bh
pointer can cause data not to be processed by the usb mass storage gadget
code.

Thanks,
Victor



================================================================================================
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information and may be protected by legal privilege. If you are not the intended addressee (or authorized to receive for the addressee). be aware that any disclosure, copying, distribution or use of this e-mail or any attachment is prohibited. If you have received this e-mail in error, please notify us immediately by returning it to the sender and delete this copy from your system. Thank you for your cooperation.
KeyASIC Inc.
================================================================================================

[U-Boot] USB mass storage gadget patch

[victor]

Lukasz,

> Lukasz,
> 
> > Hi victor,
> >
> > Regarding UMS support:
> >
> > The last version posted at u-boot ML can be found at [**]:
> > http://thread.gmane.org/gmane.comp.boot-loaders.u-
> > boot/113004/match=ums
> >
> >
> > +if (fsg_fs_bulk_out_desc.bEndpointAddress != 0x1)
> > +	fsg_fs_bulk_out_desc.bEndpointAddress = 0x1;
> > +
> >
> > If possible, please align to patch set [**], so we can have the same
setup.
> >
> > However, I will try to find some time to help you with the problem
> > (that's why I've asked for description of the problem, which you are
trying
> to fix).
> >
> 
> I hardcode bulk out endpoint to EP1 because the hardware only got one
> endpoint. So the code has to use EP1 for both bulk in and bulk out data
> transfer. Will this break the usb mass storage gadget code?
> 
> I will get the file_storage.c  from [**].
> 
> The short description of the problem:
> The U-boot usb mass storage gadget driver has problem with buffer
> management, sometimes the buffer returned by IRQ routine and the
> get_next_command function are different. It causes problem in processing
> SCSI_MODE_SELECT_6 command.
> 
> The detailed description of the problem:
> When receiving the SCSI_MODE_SELECT_6 command, in
> bulk_out_complete(), the bh pointer is 0x1b872e4. Then, in
> get_next_command(), before calling start_transfer(), the bh pointer
> becomes 0x1b87304. This changes in bh pointer can cause data not to be
> processed by the usb mass storage gadget code.
> 
> Thanks,
> Victor

I use the file_storage.c from [**]. Basically, I replace file_storage.c and
hardcode bulk out endpoint to EP1. The IRQ routine can receive the
SCSI_INQUIRY but the get_next_command() do not process it. The
get_next_command is listening on a different bh than the one returned by IRQ
routine. 

Regards,
victor


================================================================================================
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information and may be protected by legal privilege. If you are not the intended addressee (or authorized to receive for the addressee). be aware that any disclosure, copying, distribution or use of this e-mail or any attachment is prohibited. If you have received this e-mail in error, please notify us immediately by returning it to the sender and delete this copy from your system. Thank you for your cooperation.
KeyASIC Inc.
================================================================================================

[U-Boot] USB mass storage gadget patch

[victor]

Lukasz,

> -----Original Message-----
> From: victor [mailto:victor at keyasic.com]
> Sent: Tuesday, 6 November, 2012 7:09 PM
> To: 'Lukasz Majewski'
> Cc: 'u-boot at lists.denx.de'; 'Marek Vasut'; 'Kyungmin Park'
> Subject: RE: [U-Boot] USB mass storage gadget patch
> 
> Lukasz,
> 
> > Lukasz,
> >
> > > Hi victor,
> > >
> > > Regarding UMS support:
> > >
> > > The last version posted at u-boot ML can be found at [**]:
> > > http://thread.gmane.org/gmane.comp.boot-loaders.u-
> > > boot/113004/match=ums
> > >
> > >
> > > +if (fsg_fs_bulk_out_desc.bEndpointAddress != 0x1)
> > > +	fsg_fs_bulk_out_desc.bEndpointAddress = 0x1;
> > > +
> > >
> > > If possible, please align to patch set [**], so we can have the same
setup.
> > >
> > > However, I will try to find some time to help you with the problem
> > > (that's why I've asked for description of the problem, which you are
> > > trying
> > to fix).
> > >
> >
> > I hardcode bulk out endpoint to EP1 because the hardware only got one
> > endpoint. So the code has to use EP1 for both bulk in and bulk out
> > data transfer. Will this break the usb mass storage gadget code?
> >
> > I will get the file_storage.c  from [**].
> >
> > The short description of the problem:
> > The U-boot usb mass storage gadget driver has problem with buffer
> > management, sometimes the buffer returned by IRQ routine and the
> > get_next_command function are different. It causes problem in
> > processing
> > SCSI_MODE_SELECT_6 command.
> >
> > The detailed description of the problem:
> > When receiving the SCSI_MODE_SELECT_6 command, in
> bulk_out_complete(),
> > the bh pointer is 0x1b872e4. Then, in get_next_command(), before
> > calling start_transfer(), the bh pointer becomes 0x1b87304. This
> > changes in bh pointer can cause data not to be processed by the usb
> > mass storage gadget code.
> >
> > Thanks,
> > Victor
> 
> I use the file_storage.c from [**]. Basically, I replace file_storage.c
and
> hardcode bulk out endpoint to EP1. The IRQ routine can receive the
> SCSI_INQUIRY but the get_next_command() do not process it. The
> get_next_command is listening on a different bh than the one returned by
> IRQ routine.
> 
> Regards,
> victor

Any comments or opinion on the above buffer mismatch issue in usb mass
storage gadget?

Regards,
Victor


================================================================================================
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information and may be protected by legal privilege. If you are not the intended addressee (or authorized to receive for the addressee). be aware that any disclosure, copying, distribution or use of this e-mail or any attachment is prohibited. If you have received this e-mail in error, please notify us immediately by returning it to the sender and delete this copy from your system. Thank you for your cooperation.
KeyASIC Inc.
================================================================================================

[U-Boot] USB mass storage gadget patch

[victor]

>> I attach the file for your reference. Please kindly advise me. Thanks.

The changes of file_storage.c:

--- file_storage.c	2012-11-02 18:27:41.072702402 +0800
+++ file_storage.orig.c	2012-11-02 19:36:37.992528292 +0800

@@ -746,10 +745,6 @@
 	struct fsg_dev		*fsg = ep->driver_data;
 	struct fsg_buffhd	*bh = req->context;
 
-ga_bh = bh;
-ga_tag = fsg->tag;
-
 	dump_msg(fsg, "bulk-out", req->buf, req->actual);
 	if (req->status || req->actual != bh->bulk_out_intended_length)
 		DBG(fsg, "%s --> %d, %u/%u\n", __func__,

@@ -1185,9 +1174,6 @@
 				return rc;
 		}
 
-bh->inreq->buf = bh->buf;
-
 		/* If we were asked to read past the end of file,
 		 * end with an empty buffer. */
 		if (amount == 0) {
@@ -1575,8 +1560,6 @@
 	static char vendor_id[] = "Linux   ";
 	static char product_cdrom_id[] = "File-CD Gadget  ";
 
-bh->inreq->buf = bh->buf;	
 
 	if (!fsg->curlun) {		// Unsupported LUNs are okay
 		fsg->bad_lun_okay = 1;
@@ -1667,15 +1649,12 @@
 	int		pmi = fsg->cmnd[8];
 	u8		*buf = (u8 *) bh->buf;
 
-bh->inreq->buf = bh->buf;
 	/* Check the PMI and LBA fields */
 	if (pmi > 1 || (pmi == 0 && lba != 0)) {
 		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
 		return -EINVAL;
 	}
 
@@ -1755,8 +1734,6 @@
 		curlun->sense_data = SS_SAVING_PARAMETERS_NOT_SUPPORTED;
 		return -EINVAL;
 	}
-printf("C %s %x ", __func__, (u8 *)bh->buf);
-bh->inreq->buf = bh->buf;
 
 	changeable_values = (pc == 1);
 	all_pages = (page_code == 0x3f);
@@ -1865,8 +1842,6 @@
 	struct fsg_lun	*curlun = fsg->curlun;
 	u8		*buf = (u8 *) bh->buf;
 
-printf("%s\n",__func__);
-bh->inreq->buf = bh->buf;
 	buf[0] = buf[1] = buf[2] = 0;
 	buf[3] = 8;		// Only the Current/Maximum Capacity
Descriptor
 	buf += 4;
@@ -1956,9 +1931,8 @@
 
 		nsend = min(fsg->usb_amount_left, (u32) mod_data.buflen);
 		memset(bh->buf + nkeep, 0, nsend - nkeep);
-		//bh->inreq->length = nsend;
+		bh->inreq->length = nsend;
 		bh->inreq->zero = 0;
-printf("%s %x %d %d\n", __func__, (u8 *)bh, bh->inreq->length, nsend);
 		start_transfer(fsg, fsg->bulk_in, bh->inreq,
 				&bh->inreq_busy, &bh->state);
 		bh = fsg->next_buffhd_to_fill = bh->next;

@@ -2137,13 +2107,7 @@
 
 	/* Wait for the next buffer to become available */
 	bh = fsg->next_buffhd_to_fill;
-printf("%s %x %d\n", __func__,  (u8 *)bh, bh->state);
 	while (bh->state != BUF_STATE_EMPTY) {
-if (bh->state == BUF_STATE_FULL)
-{
-	bh->state = BUF_STATE_EMPTY;
-	break;
-}
 		rc = sleep_thread(fsg, __LINE__);
 		if (rc)
 			return rc;
@@ -2180,8 +2144,6 @@
 
 		bh->inreq->length = USB_BULK_CS_WRAP_LEN;
 		bh->inreq->zero = 0;
-bh->inreq->buf = bh->buf;
-printf("bbb %s %x %d\n", __func__, (u8 *)bh, bh->state);
 		start_transfer(fsg, fsg->bulk_in, bh->inreq,
 				&bh->inreq_busy, &bh->state);
 

@@ -2362,12 +2320,9 @@
 	for (i = 1; i < cmnd_size; ++i) {
 		if (fsg->cmnd[i] && !(mask & (1 << i))) {
 
			if (curlun)
 				curlun->sense_data =
SS_INVALID_FIELD_IN_CDB;
			//return -EINVAL;

 		}
 	}
			
@@ -2669,8 +2620,6 @@
 
                 ga_bh->inreq->length = USB_BULK_CS_WRAP_LEN;
                 ga_bh->inreq->zero = 0;
-ga_bh->inreq->buf = ga_bh->buf;
-printf("bbb %s %x %d\n", __func__, (u8 *)ga_bh, ga_bh->state);
                 start_transfer(fsg, fsg->bulk_in, ga_bh->inreq,
                                 &(ga_bh->inreq_busy), &(ga_bh->state));
 
@@ -2732,26 +2679,20 @@
 		/* Queue a request to read a Bulk-only CBW */
 		set_bulk_out_req_length(fsg, bh, USB_BULK_CB_WRAP_LEN);
 		bh->outreq->short_not_ok = 1;
-printf("call start_transfer, %x %x\n", (u8 *)bh, (u8 *)(fsg->bulk_out));
-bh->outreq->context = bh;
 		start_transfer(fsg, fsg->bulk_out, bh->outreq,
 				&bh->outreq_busy, &bh->state);
 		
@@ -3468,8 +3406,6 @@
 	ep->driver_data = fsg;		// claim the endpoint
 	fsg->bulk_out = ep;
 
-if (fsg_fs_bulk_out_desc.bEndpointAddress != 0x1)
-	fsg_fs_bulk_out_desc.bEndpointAddress = 0x1;
 
 	if (transport_is_cbi()) {
 		ep = usb_ep_autoconfig(gadget, &fsg_fs_intr_in_desc);


================================================================================================
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information and may be protected by legal privilege. If you are not the intended addressee (or authorized to receive for the addressee). be aware that any disclosure, copying, distribution or use of this e-mail or any attachment is prohibited. If you have received this e-mail in error, please notify us immediately by returning it to the sender and delete this copy from your system. Thank you for your cooperation.
KeyASIC Inc.
================================================================================================