Message ID | 20181212143135.29147-3-lvivier@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | usb/storage: Implement block write support | expand |
On 2018-12-12 15:31, Laurent Vivier wrote: > The only missing parts were to manage the transfer direction in > do-bulk-command and to copy the data to the buffer before the > write operation. > > This is needed as GRUB2 wants to write the grubenv file at start > and hangs because the data are not provided to the disk controller. > > I've checked the file is correctly modified by modifying an environment > variable in GRUB2 with "set saved_entry=2" then "save_env saved_entry" > and checking the result in linux with "grub2-editenv list". > > Fixes: Fixes: a0b96fe66fcd991b407c1d67ca842921e477a6fd > (Provide "write" function in the disk-label package) > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > slof/fs/usb/dev-storage.fs | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/slof/fs/usb/dev-storage.fs b/slof/fs/usb/dev-storage.fs > index fe5af1c..449659e 100644 > --- a/slof/fs/usb/dev-storage.fs > +++ b/slof/fs/usb/dev-storage.fs > @@ -103,7 +103,7 @@ scsi-open > \ if sense-len is 0 then no sense data is actually present > \ > > -: do-bulk-command ( resp-buffer resp-size -- TRUE | FALSE ) > +: do-bulk-command ( dir resp-buffer resp-size -- TRUE | FALSE ) > TO resp-size > TO resp-buffer > udev USB_PIPE_OUT td-buf td-buf-phys dma-buf-phys usb>cmd 1F > @@ -113,13 +113,20 @@ scsi-open > \ transfer CBW > resp-size IF > d# 125 us > - udev USB_PIPE_IN td-buf td-buf-phys resp-buffer resp-size > + IF > + udev USB_PIPE_IN > + ELSE > + udev USB_PIPE_OUT > + THEN > + td-buf td-buf-phys resp-buffer resp-size > usb-transfer-bulk 1 = not IF \ transfer data > usb-disk-debug? IF ." Data phase failed " cr THEN > \ FALSE EXIT > \ in case of a stall/halted endpoint we clear the halt > \ Fall through and try reading the CSW > THEN > + ELSE > + drop > THEN > d# 125 us > udev USB_PIPE_IN td-buf td-buf-phys dma-buf-phys usb>csw 0D There is another "ELSE FALSE EXIT THEN" at the end of this function. I think you've got to drop the "dir" stack item in that case, too, don't you? Thomas
On Wed, Dec 12, 2018 at 09:14:21PM +0100, Thomas Huth wrote: > On 2018-12-12 15:31, Laurent Vivier wrote: > There is another "ELSE FALSE EXIT THEN" at the end of this function. I > think you've got to drop the "dir" stack item in that case, too, don't you? Early exits should be early, not late. That's the point of early exits: it reduces nesting level, and confusion about what is on the stacks when. Segher
On 2018-12-12 22:10, Segher Boessenkool wrote: > On Wed, Dec 12, 2018 at 09:14:21PM +0100, Thomas Huth wrote: >> On 2018-12-12 15:31, Laurent Vivier wrote: >> There is another "ELSE FALSE EXIT THEN" at the end of this function. I >> think you've got to drop the "dir" stack item in that case, too, don't you? > > Early exits should be early, not late. That's the point of early exits: > it reduces nesting level, and confusion about what is on the stacks when. Yeah, it's early now, Laurent cleaned it up by moving it to the beginning in the first patch - I just mixed it up here again while looking at the original source code when I reviewed the second patch. Everything is fine in v4 of the series. Thomas
On 12/12/2018 22:10, Segher Boessenkool wrote: > On Wed, Dec 12, 2018 at 09:14:21PM +0100, Thomas Huth wrote: >> On 2018-12-12 15:31, Laurent Vivier wrote: >> There is another "ELSE FALSE EXIT THEN" at the end of this function. I >> think you've got to drop the "dir" stack item in that case, too, don't you? > > Early exits should be early, not late. That's the point of early exits: > it reduces nesting level, and confusion about what is on the stacks when. In fact, the "EXIT" Thomas is speaking about is at the top of the function, it's why we have to drop the "dir" stack item: it has not been used. The latest version of the series is v4. Thanks, Laurent
On Thu, Dec 13, 2018 at 09:13:55AM +0100, Laurent Vivier wrote: > On 12/12/2018 22:10, Segher Boessenkool wrote: > > On Wed, Dec 12, 2018 at 09:14:21PM +0100, Thomas Huth wrote: > >> On 2018-12-12 15:31, Laurent Vivier wrote: > >> There is another "ELSE FALSE EXIT THEN" at the end of this function. I > >> think you've got to drop the "dir" stack item in that case, too, don't you? > > > > Early exits should be early, not late. That's the point of early exits: > > it reduces nesting level, and confusion about what is on the stacks when. > > In fact, the "EXIT" Thomas is speaking about is at the top of the > function, it's why we have to drop the "dir" stack item: it has not been > used. The latest version of the series is v4. For some reason the v2 showed up as newer in my mail. Dunno :-) Segher
diff --git a/slof/fs/usb/dev-storage.fs b/slof/fs/usb/dev-storage.fs index fe5af1c..449659e 100644 --- a/slof/fs/usb/dev-storage.fs +++ b/slof/fs/usb/dev-storage.fs @@ -103,7 +103,7 @@ scsi-open \ if sense-len is 0 then no sense data is actually present \ -: do-bulk-command ( resp-buffer resp-size -- TRUE | FALSE ) +: do-bulk-command ( dir resp-buffer resp-size -- TRUE | FALSE ) TO resp-size TO resp-buffer udev USB_PIPE_OUT td-buf td-buf-phys dma-buf-phys usb>cmd 1F @@ -113,13 +113,20 @@ scsi-open \ transfer CBW resp-size IF d# 125 us - udev USB_PIPE_IN td-buf td-buf-phys resp-buffer resp-size + IF + udev USB_PIPE_IN + ELSE + udev USB_PIPE_OUT + THEN + td-buf td-buf-phys resp-buffer resp-size usb-transfer-bulk 1 = not IF \ transfer data usb-disk-debug? IF ." Data phase failed " cr THEN \ FALSE EXIT \ in case of a stall/halted endpoint we clear the halt \ Fall through and try reading the CSW THEN + ELSE + drop THEN d# 125 us udev USB_PIPE_IN td-buf td-buf-phys dma-buf-phys usb>csw 0D @@ -182,18 +189,28 @@ CONSTANT cbw-length build-cbw 1 tag + to tag + \ copy command usb-cmd-addr dma-buf usb>cmd SCSI-COMMAND-OFFSET + usb-cmd-len move + \ copy data to write + usb-dir not IF + usb-buf-addr dma-buf usb>data usb-buf-len move + THEN + \ Send it - dma-buf-phys usb>data usb-buf-len + usb-dir dma-buf-phys usb>data usb-buf-len do-bulk-command 1 = not IF ." USB-DISK: Bulk command failed!" cr 0 0 -1 EXIT THEN - dma-buf usb>data usb-buf-addr usb-buf-len move + + \ copy read data + usb-dir IF + dma-buf usb>data usb-buf-addr usb-buf-len move + THEN dma-buf usb>csw to csw-addr csw-addr csw>sig l@ 55534253 <> IF
The only missing parts were to manage the transfer direction in do-bulk-command and to copy the data to the buffer before the write operation. This is needed as GRUB2 wants to write the grubenv file at start and hangs because the data are not provided to the disk controller. I've checked the file is correctly modified by modifying an environment variable in GRUB2 with "set saved_entry=2" then "save_env saved_entry" and checking the result in linux with "grub2-editenv list". Fixes: Fixes: a0b96fe66fcd991b407c1d67ca842921e477a6fd (Provide "write" function in the disk-label package) Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- slof/fs/usb/dev-storage.fs | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)