| Message ID | 20181212202344.14818-3-lvivier@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | usb/storage: Implement block write support | expand |
On 2018-12-12 21:23, 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 | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/slof/fs/usb/dev-storage.fs b/slof/fs/usb/dev-storage.fs > index a0a0bac..db5d0a8 100644 > --- a/slof/fs/usb/dev-storage.fs > +++ b/slof/fs/usb/dev-storage.fs > @@ -103,23 +103,30 @@ 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 > usb-transfer-bulk 0= IF > - FALSE EXIT > + drop FALSE EXIT > THEN > \ 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 0= 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 0= 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 > LGTM now, thanks! Reviewed-by: Thomas Huth <thuth@redhat.com>
On 13/12/2018 07:23, 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) Thanks for the patches, I applied them. But the commit log confuses me. The "fixes" patch added forwarding of "write" to the parent in disk-label, how does this one fix it? Why only usb? scsi does it right already? > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > slof/fs/usb/dev-storage.fs | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/slof/fs/usb/dev-storage.fs b/slof/fs/usb/dev-storage.fs > index a0a0bac..db5d0a8 100644 > --- a/slof/fs/usb/dev-storage.fs > +++ b/slof/fs/usb/dev-storage.fs > @@ -103,23 +103,30 @@ 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 > usb-transfer-bulk 0= IF > - FALSE EXIT > + drop FALSE EXIT > THEN > \ 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 0= 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 0= 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 >
On 20/12/2018 07:57, Alexey Kardashevskiy wrote: > > > On 13/12/2018 07:23, 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) > > Thanks for the patches, I applied them. Thank you. > But the commit log confuses me. > > The "fixes" patch added forwarding of "write" to the parent in > disk-label, how does this one fix it? > > Why only usb? scsi does it right already? > All is fine with SCSI, the problem was at the USB storage driver level. It fixes it because the "write" was forwarded to the USB storage driver and the USB storage driver doesn't support it and GRUB2 fails to boot. Without Thomas' patch, GRUB2 cannot write but there is a clean error and it can load the kernel and boot it. Thank you, Laurent
diff --git a/slof/fs/usb/dev-storage.fs b/slof/fs/usb/dev-storage.fs index a0a0bac..db5d0a8 100644 --- a/slof/fs/usb/dev-storage.fs +++ b/slof/fs/usb/dev-storage.fs @@ -103,23 +103,30 @@ 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 usb-transfer-bulk 0= IF - FALSE EXIT + drop FALSE EXIT THEN \ 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 0= 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 0= 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 | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)