* [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs @ 2022-04-06 9:24 Maxim Devaev 2022-04-06 15:24 ` Alan Stern 0 siblings, 1 reply; 17+ messages in thread From: Maxim Devaev @ 2022-04-06 9:24 UTC (permalink / raw) To: linux-usb Cc: mdevaev, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel Using the SIGUSR1 signal sent to the "file-storage" thread from the userspace, it is possible to break IO operations that block the gadget. Thus, it is possible to implement "force eject" without stopping the gadget and umounting it from the host side. There are two problems here: - In order to send a signal, we need to find the thread in procfs, but if several mass storage gadgets are created in the system, each process has the same name and it is impossible to distinguish one gadget from another. - Root privileges are required to send the signal. The proposed "break_io" interface solves both problems. It allows us to get rid of the procfs search and delegate sending the signal to a regular user. Signed-off-by: Maxim Devaev <mdevaev@gmail.com> --- drivers/usb/gadget/function/f_mass_storage.c | 22 ++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index 6ad669dde41c..e9b7c59e1dc4 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -3239,6 +3239,27 @@ static ssize_t fsg_opts_stall_store(struct config_item *item, const char *page, CONFIGFS_ATTR(fsg_opts_, stall); +static ssize_t fsg_opts_break_io_store(struct config_item *item, + const char *page, size_t len) +{ + struct fsg_opts *opts = to_fsg_opts(item); + unsigned long flags; + + mutex_lock(&opts->lock); + spin_lock_irqsave(&opts->common->lock, flags); + + if (opts->common->thread_task) + send_sig_info(SIGUSR1, SEND_SIG_PRIV, + opts->common->thread_task); + + spin_unlock_irqrestore(&opts->common->lock, flags); + mutex_unlock(&opts->lock); + + return len; +} + +CONFIGFS_ATTR_WO(fsg_opts_, break_io); + #ifdef CONFIG_USB_GADGET_DEBUG_FILES static ssize_t fsg_opts_num_buffers_show(struct config_item *item, char *page) { @@ -3283,6 +3304,7 @@ CONFIGFS_ATTR(fsg_opts_, num_buffers); static struct configfs_attribute *fsg_attrs[] = { &fsg_opts_attr_stall, + &fsg_opts_attr_break_io, #ifdef CONFIG_USB_GADGET_DEBUG_FILES &fsg_opts_attr_num_buffers, #endif -- 2.35.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-06 9:24 [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs Maxim Devaev @ 2022-04-06 15:24 ` Alan Stern 2022-04-06 16:52 ` Maxim Devaev 0 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2022-04-06 15:24 UTC (permalink / raw) To: Maxim Devaev Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel On Wed, Apr 06, 2022 at 12:24:45PM +0300, Maxim Devaev wrote: > Using the SIGUSR1 signal sent to the "file-storage" thread > from the userspace, it is possible to break IO operations > that block the gadget. Thus, it is possible to implement > "force eject" without stopping the gadget and umounting > it from the host side. It's not clear to me how breaking I/O operations allows you to do a "force eject". It seems that what you would need is something like fsg_store_file() that omits the curlun->prevent_medium_removal check. Interrupting a lengthy I/O operation doesn't really have anything to do with this. > There are two problems here: > > - In order to send a signal, we need to find the thread > in procfs, but if several mass storage gadgets are created > in the system, each process has the same name and it is > impossible to distinguish one gadget from another. > > - Root privileges are required to send the signal. > > The proposed "break_io" interface solves both problems. > It allows us to get rid of the procfs search and delegate > sending the signal to a regular user. Or to keep this ability restricted to the superuser, if that is desired. > Signed-off-by: Maxim Devaev <mdevaev@gmail.com> > --- > drivers/usb/gadget/function/f_mass_storage.c | 22 ++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > index 6ad669dde41c..e9b7c59e1dc4 100644 > --- a/drivers/usb/gadget/function/f_mass_storage.c > +++ b/drivers/usb/gadget/function/f_mass_storage.c > @@ -3239,6 +3239,27 @@ static ssize_t fsg_opts_stall_store(struct config_item *item, const char *page, > > CONFIGFS_ATTR(fsg_opts_, stall); > > +static ssize_t fsg_opts_break_io_store(struct config_item *item, > + const char *page, size_t len) > +{ > + struct fsg_opts *opts = to_fsg_opts(item); > + unsigned long flags; > + > + mutex_lock(&opts->lock); > + spin_lock_irqsave(&opts->common->lock, flags); > + > + if (opts->common->thread_task) > + send_sig_info(SIGUSR1, SEND_SIG_PRIV, > + opts->common->thread_task); You should not call send_sig_info() directly; instead call raise_exception(). It already does the work you need (including some things you left out). Alan Stern > + > + spin_unlock_irqrestore(&opts->common->lock, flags); > + mutex_unlock(&opts->lock); > + > + return len; > +} > + > +CONFIGFS_ATTR_WO(fsg_opts_, break_io); > + > #ifdef CONFIG_USB_GADGET_DEBUG_FILES > static ssize_t fsg_opts_num_buffers_show(struct config_item *item, char *page) > { > @@ -3283,6 +3304,7 @@ CONFIGFS_ATTR(fsg_opts_, num_buffers); > > static struct configfs_attribute *fsg_attrs[] = { > &fsg_opts_attr_stall, > + &fsg_opts_attr_break_io, > #ifdef CONFIG_USB_GADGET_DEBUG_FILES > &fsg_opts_attr_num_buffers, > #endif > -- > 2.35.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-06 15:24 ` Alan Stern @ 2022-04-06 16:52 ` Maxim Devaev 2022-04-06 17:51 ` Alan Stern 0 siblings, 1 reply; 17+ messages in thread From: Maxim Devaev @ 2022-04-06 16:52 UTC (permalink / raw) To: Alan Stern Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel > It's not clear to me how breaking I/O operations allows you to do a > "force eject". It seems that what you would need is something like > fsg_store_file() that omits the curlun->prevent_medium_removal check. > Interrupting a lengthy I/O operation doesn't really have anything to do > with this. Perhaps I chose the wrong path, it's just how my userspace code works now. If the drive is connected to a Linux host, then in order to clear the "file" and extract the image, I sent a SIGUSR1 signal to the "file-storage" thread. This interrupted long IO operations, reset curlun->prevent_medium_removal and I got the ability to extract. It was done in our KVM-over-IP project and worked for several years, just now I want to do it without searching for procfs and the need to use sudo helpers like this: https://github.com/pikvm/kvmd/blob/1b3a2cc/kvmd/helpers/otgmsd/unlock/__init__.py Maybe it's worth introducing some option that will allow us to ignore curlun->prevent_medium_removal and perform a forced extraction? Something like "allow_force_eject" on the same lavel with "stall". Will masking the curlun->prevent_medium_removal flag be enough? > Or to keep this ability restricted to the superuser, if that is desired. Indeed. > You should not call send_sig_info() directly; instead call > raise_exception(). It already does the work you need (including some > things you left out). raise_exception() assumes the setting of a new state, and I did not want to do this, since the same does not happen when throwing a signal from userspace. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-06 16:52 ` Maxim Devaev @ 2022-04-06 17:51 ` Alan Stern 2022-04-06 18:36 ` Maxim Devaev 0 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2022-04-06 17:51 UTC (permalink / raw) To: Maxim Devaev Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel On Wed, Apr 06, 2022 at 07:52:34PM +0300, Maxim Devaev wrote: > > It's not clear to me how breaking I/O operations allows you to do a > > "force eject". It seems that what you would need is something like > > fsg_store_file() that omits the curlun->prevent_medium_removal check. > > Interrupting a lengthy I/O operation doesn't really have anything to do > > with this. > > Perhaps I chose the wrong path, it's just how my userspace code works now. > If the drive is connected to a Linux host, then in order to clear > the "file" and extract the image, I sent a SIGUSR1 signal to the "file-storage" > thread. This interrupted long IO operations, reset curlun->prevent_medium_removal > and I got the ability to extract. Oh, I see. That's kind of an unintended side effect of not calling raise_exception(). And while it does interrupt long I/O operations, it does so in non-sanctioned way. To the host it will appear as though the gadget's firmware has crashed, since the gadget will stop sending or receiving data. Eventually the host will time out and reset the gadget. Maybe that's the sort of thing you want, but I rather doubt it. > It was done in our KVM-over-IP project and worked for several years, > just now I want to do it without searching for procfs and the need > to use sudo helpers like this: > https://github.com/pikvm/kvmd/blob/1b3a2cc/kvmd/helpers/otgmsd/unlock/__init__.py > > Maybe it's worth introducing some option that will allow us to ignore > curlun->prevent_medium_removal and perform a forced extraction? > Something like "allow_force_eject" on the same lavel with "stall". Or have a separate sysfs file where any write at all will be interpreted as a forced eject. Either way would work. > Will masking the curlun->prevent_medium_removal flag be enough? I think so. But it will be blocked to some extent by long-running I/O operations, because those operations acquire the filesem rw-semaphore for reading. More precisely, each individual command holds the rw-semaphore. But the semaphore is dropped between commands, and a long-running I/O operation typically consists of many separate commands. So the blocking may be acceptable. > > Or to keep this ability restricted to the superuser, if that is desired. > > Indeed. > > > You should not call send_sig_info() directly; instead call > > raise_exception(). It already does the work you need (including some > > things you left out). > > raise_exception() assumes the setting of a new state, and I did not want to do this, > since the same does not happen when throwing a signal from userspace. Userspace isn't supposed to send the USR1 signal, only the INT, TERM, or KILL signals. USR1 is supposed to be reserved for the driver's internal use. Unfortunately, AFAIK there's no way to allow the driver to send a signal to itself without also allowing the signal to be sent by userspace. :-( And sending the signal _does_ set a new state, whether you intended to or not. Although in this case, the new state is always the same as the old state, i.e., FSG_STATE_NORMAL. Alan Stern ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-06 17:51 ` Alan Stern @ 2022-04-06 18:36 ` Maxim Devaev 2022-04-07 16:06 ` Alan Stern 0 siblings, 1 reply; 17+ messages in thread From: Maxim Devaev @ 2022-04-06 18:36 UTC (permalink / raw) To: Alan Stern Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel В Wed, 6 Apr 2022 13:51:40 -0400 Alan Stern <stern@rowland.harvard.edu> wrote: > On Wed, Apr 06, 2022 at 07:52:34PM +0300, Maxim Devaev wrote: > > > It's not clear to me how breaking I/O operations allows you to do a > > > "force eject". It seems that what you would need is something like > > > fsg_store_file() that omits the curlun->prevent_medium_removal check. > > > Interrupting a lengthy I/O operation doesn't really have anything to do > > > with this. > > > > Perhaps I chose the wrong path, it's just how my userspace code works now. > > If the drive is connected to a Linux host, then in order to clear > > the "file" and extract the image, I sent a SIGUSR1 signal to the "file-storage" > > thread. This interrupted long IO operations, reset curlun->prevent_medium_removal > > and I got the ability to extract. > > Oh, I see. That's kind of an unintended side effect of not calling > raise_exception(). > > And while it does interrupt long I/O operations, it does so in > non-sanctioned way. To the host it will appear as though the gadget's > firmware has crashed, since the gadget will stop sending or receiving > data. Eventually the host will time out and reset the gadget. > > Maybe that's the sort of thing you want, but I rather doubt it. It's hard to say how it actually should work in case of force removing. At least the currect approach with SIGUSR1 is really working on thousands systems and with Linux, Mac and Windows. I believe that the criterion of the experiment is quite important here. I know of several other utilities that use SIGUSR1 for similar purposes. > > Will masking the curlun->prevent_medium_removal flag be enough? > > I think so. But it will be blocked to some extent by long-running I/O > operations, because those operations acquire the filesem rw-semaphore > for reading. > > More precisely, each individual command holds the rw-semaphore. But the > semaphore is dropped between commands, and a long-running I/O operation > typically consists of many separate commands. So the blocking may be > acceptable. It is very important for KVM-over-IP to be able to command "turn it off immediately". In this context, I would prefer "break_io" rather than "allow_force_remove". > > > You should not call send_sig_info() directly; instead call > > > raise_exception(). It already does the work you need (including some > > > things you left out). > > > > raise_exception() assumes the setting of a new state, and I did not want to do this, > > since the same does not happen when throwing a signal from userspace. > > Userspace isn't supposed to send the USR1 signal, only the INT, TERM, or > KILL signals. USR1 is supposed to be reserved for the driver's internal > use. Unfortunately, AFAIK there's no way to allow the driver to send a > signal to itself without also allowing the signal to be sent by > userspace. :-( It's funny that you actually helped me solve my problem thanks to this undocumented behavior. If it were not for the ability to send a signal, I would not be able to make the necessary code, and my software would always be waiting for the completion of IO. So here I am grateful to you - I didn't have to patch the kernel a few years ago, and now I just want to turn it into a clear feature :) Given the needs of the userspace code, maybe the suggested "break_io" would be the best choice? > And sending the signal _does_ set a new state, whether you intended to > or not. Although in this case, the new state is always the same as the > old state, i.e., FSG_STATE_NORMAL. So I could call raise_exception(fsg->common, FSG_STATE_NORMAL) instead of sending the signal from break_io handler. There will be a slight difference in exception_req_tag and exception_arg, but it does not seem to cause any side effects. Please correct me if I'm wrong. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-06 18:36 ` Maxim Devaev @ 2022-04-07 16:06 ` Alan Stern 2022-04-07 17:47 ` Maxim Devaev 0 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2022-04-07 16:06 UTC (permalink / raw) To: Maxim Devaev Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel On Wed, Apr 06, 2022 at 09:36:34PM +0300, Maxim Devaev wrote: > В Wed, 6 Apr 2022 13:51:40 -0400 > Alan Stern <stern@rowland.harvard.edu> wrote: > > > On Wed, Apr 06, 2022 at 07:52:34PM +0300, Maxim Devaev wrote: > > > > It's not clear to me how breaking I/O operations allows you to do a > > > > "force eject". It seems that what you would need is something like > > > > fsg_store_file() that omits the curlun->prevent_medium_removal check. > > > > Interrupting a lengthy I/O operation doesn't really have anything to do > > > > with this. > > > > > > Perhaps I chose the wrong path, it's just how my userspace code works now. > > > If the drive is connected to a Linux host, then in order to clear > > > the "file" and extract the image, I sent a SIGUSR1 signal to the "file-storage" > > > thread. This interrupted long IO operations, reset curlun->prevent_medium_removal > > > and I got the ability to extract. > > > > Oh, I see. That's kind of an unintended side effect of not calling > > raise_exception(). > > > > And while it does interrupt long I/O operations, it does so in > > non-sanctioned way. To the host it will appear as though the gadget's > > firmware has crashed, since the gadget will stop sending or receiving > > data. Eventually the host will time out and reset the gadget. > > > > Maybe that's the sort of thing you want, but I rather doubt it. > > It's hard to say how it actually should work in case of force removing. > At least the currect approach with SIGUSR1 is really working on thousands > systems and with Linux, Mac and Windows. I believe that the criterion > of the experiment is quite important here. I know of several other utilities > that use SIGUSR1 for similar purposes. This merely means that the current unintended behavior of userspace USR1 signals must not be changed. But it doesn't mean you have to continue to rely on that behavior; you can implement something better. > > > Will masking the curlun->prevent_medium_removal flag be enough? > > > > I think so. But it will be blocked to some extent by long-running I/O > > operations, because those operations acquire the filesem rw-semaphore > > for reading. > > > > More precisely, each individual command holds the rw-semaphore. But the > > semaphore is dropped between commands, and a long-running I/O operation > > typically consists of many separate commands. So the blocking may be > > acceptable. > > It is very important for KVM-over-IP to be able to command "turn it off immediately". Why is this? A lot of actual devices (DVD drives, for instance) don't give you the ability to eject the media when the host has prevented it. Why should f-mass-storage be different? > In this context, I would prefer "break_io" rather than "allow_force_remove". Okay. But what about the 30-second host timeout I mentioned above? Does this actually happen with your approach? It seems like the kind of thing you don't want in a "turn it off immediately" situation. (I haven't tried doing this myself -- maybe I should.) > > > > You should not call send_sig_info() directly; instead call > > > > raise_exception(). It already does the work you need (including some > > > > things you left out). > > > > > > raise_exception() assumes the setting of a new state, and I did not want to do this, > > > since the same does not happen when throwing a signal from userspace. > > > > Userspace isn't supposed to send the USR1 signal, only the INT, TERM, or > > KILL signals. USR1 is supposed to be reserved for the driver's internal > > use. Unfortunately, AFAIK there's no way to allow the driver to send a > > signal to itself without also allowing the signal to be sent by > > userspace. :-( > > It's funny that you actually helped me solve my problem thanks to this undocumented > behavior. If it were not for the ability to send a signal, I would not be able to make > the necessary code, and my software would always be waiting for the completion of IO. > > So here I am grateful to you - I didn't have to patch the kernel a few years ago, > and now I just want to turn it into a clear feature :) > > Given the needs of the userspace code, maybe the suggested "break_io" > would be the best choice? It sounds like what you really want is a combination of both "interrupt I/O" and "forced eject". > > And sending the signal _does_ set a new state, whether you intended to > > or not. Although in this case, the new state is always the same as the > > old state, i.e., FSG_STATE_NORMAL. > > So I could call raise_exception(fsg->common, FSG_STATE_NORMAL) instead of sending > the signal from break_io handler. There will be a slight difference > in exception_req_tag and exception_arg, but it does not seem to cause any side effects. > Please correct me if I'm wrong. In fact, the best approach would be to introduce a new state (let's call it FSG_STATE_FORCED_EJECT) with priority just above FSG_STATE_ABORT_BULK_OUT. You would call raise_exception with FSG_STATE_FORCED_EJECT, not FSG_STATE_NORMAL. handle_exceptions() would treat this state partially like ABORT_BULK_OUT in that it would avoid resetting all the LUN data values and would call send_status_common() if a command had been underway. But in addition it would do the forced eject. Also, the sysfs routine should be careful to see whether the command currently being executed is for the LUN being ejected. I guess you have never tried issuing your USR1 signal to a mass-storage gadget running more than one LUN. If you did, you would find that it clears the prevent_medium_removal flag for all of them, not just the one that you wanted. Alan Stern ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-07 16:06 ` Alan Stern @ 2022-04-07 17:47 ` Maxim Devaev 2022-04-08 14:59 ` Alan Stern 0 siblings, 1 reply; 17+ messages in thread From: Maxim Devaev @ 2022-04-07 17:47 UTC (permalink / raw) To: Alan Stern Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel В Thu, 7 Apr 2022 12:06:01 -0400 Alan Stern <stern@rowland.harvard.edu> wrote: > On Wed, Apr 06, 2022 at 09:36:34PM +0300, Maxim Devaev wrote: > > В Wed, 6 Apr 2022 13:51:40 -0400 > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > On Wed, Apr 06, 2022 at 07:52:34PM +0300, Maxim Devaev wrote: > > > > > It's not clear to me how breaking I/O operations allows you to do a > > > > > "force eject". It seems that what you would need is something like > > > > > fsg_store_file() that omits the curlun->prevent_medium_removal check. > > > > > Interrupting a lengthy I/O operation doesn't really have anything to do > > > > > with this. > > > > > > > > Perhaps I chose the wrong path, it's just how my userspace code works now. > > > > If the drive is connected to a Linux host, then in order to clear > > > > the "file" and extract the image, I sent a SIGUSR1 signal to the "file-storage" > > > > thread. This interrupted long IO operations, reset curlun->prevent_medium_removal > > > > and I got the ability to extract. > > > > > > Oh, I see. That's kind of an unintended side effect of not calling > > > raise_exception(). > > > > > > And while it does interrupt long I/O operations, it does so in > > > non-sanctioned way. To the host it will appear as though the gadget's > > > firmware has crashed, since the gadget will stop sending or receiving > > > data. Eventually the host will time out and reset the gadget. > > > > > > Maybe that's the sort of thing you want, but I rather doubt it. > > > > It's hard to say how it actually should work in case of force removing. > > At least the currect approach with SIGUSR1 is really working on thousands > > systems and with Linux, Mac and Windows. I believe that the criterion > > of the experiment is quite important here. I know of several other utilities > > that use SIGUSR1 for similar purposes. > > This merely means that the current unintended behavior of userspace USR1 > signals must not be changed. But it doesn't mean you have to continue > to rely on that behavior; you can implement something better. So I suggest break_io :) I haven't come up with anything better. > > > > Will masking the curlun->prevent_medium_removal flag be enough? > > > > > > I think so. But it will be blocked to some extent by long-running I/O > > > operations, because those operations acquire the filesem rw-semaphore > > > for reading. > > > > > > More precisely, each individual command holds the rw-semaphore. But the > > > semaphore is dropped between commands, and a long-running I/O operation > > > typically consists of many separate commands. So the blocking may be > > > acceptable. > > > > It is very important for KVM-over-IP to be able to command "turn it off immediately". > > Why is this? A lot of actual devices (DVD drives, for instance) don't > give you the ability to eject the media when the host has prevented it. > Why should f-mass-storage be different? The DVD drive has the ability to physically eject the disc. It's not too good for the drive itself, but it's just there. We can also urgently remove the USB flash drive. At least there is one situation where the behavior of f_mass_storage differs from the behavior of a real drive. What happens when you click on the physical "eject" button? Yes, the OS can block this, but the problem is that we don't have an "eject" here. If I connect the gadget to the Linux host and don't even mount the image, Linux won't let me change the image in the "file", since the gadget will be constantly busy with some IO. But I believe creating a virtual "eject" button is a separate task that does not depend on "break_io". > > In this context, I would prefer "break_io" rather than "allow_force_remove". > > Okay. But what about the 30-second host timeout I mentioned above? > Does this actually happen with your approach? It seems like the kind of > thing you don't want in a "turn it off immediately" situation. (I > haven't tried doing this myself -- maybe I should.) Neither I nor my users noticed any problems related to this. After extracting the image using SIGUSR1/"file", I can just assign a new "file"image and everything will work. > > > > > You should not call send_sig_info() directly; instead call > > > > > raise_exception(). It already does the work you need (including some > > > > > things you left out). > > > > > > > > raise_exception() assumes the setting of a new state, and I did not want to do this, > > > > since the same does not happen when throwing a signal from userspace. > > > > > > Userspace isn't supposed to send the USR1 signal, only the INT, TERM, or > > > KILL signals. USR1 is supposed to be reserved for the driver's internal > > > use. Unfortunately, AFAIK there's no way to allow the driver to send a > > > signal to itself without also allowing the signal to be sent by > > > userspace. :-( > > > > It's funny that you actually helped me solve my problem thanks to this undocumented > > behavior. If it were not for the ability to send a signal, I would not be able to make > > the necessary code, and my software would always be waiting for the completion of IO. > > > > So here I am grateful to you - I didn't have to patch the kernel a few years ago, > > and now I just want to turn it into a clear feature :) > > > > Given the needs of the userspace code, maybe the suggested "break_io" > > would be the best choice? > > It sounds like what you really want is a combination of both "interrupt > I/O" and "forced eject". Indeed. But I didn't want to introduce some complex entities into the "file" attribute or make magic prefixes for the image name or something. So I suggested "echo > break_io && echo > file". This will not break the current behavior of the drive. > > > And sending the signal _does_ set a new state, whether you intended to > > > or not. Although in this case, the new state is always the same as the > > > old state, i.e., FSG_STATE_NORMAL. > > > > So I could call raise_exception(fsg->common, FSG_STATE_NORMAL) instead of sending > > the signal from break_io handler. There will be a slight difference > > in exception_req_tag and exception_arg, but it does not seem to cause any side effects. > > Please correct me if I'm wrong. > > In fact, the best approach would be to introduce a new state (let's call > it FSG_STATE_FORCED_EJECT) with priority just above > FSG_STATE_ABORT_BULK_OUT. You would call raise_exception with > FSG_STATE_FORCED_EJECT, not FSG_STATE_NORMAL. handle_exceptions() would > treat this state partially like ABORT_BULK_OUT in that it would avoid > resetting all the LUN data values and would call send_status_common() if > a command had been underway. But in addition it would do the forced > eject. Do you mean something like this? if (old_state != FSG_STATE_ABORT_BULK_OUT) { for (i = 0; i < ARRAY_SIZE(common->luns); ++i) { curlun = common->luns[i]; if (!curlun) continue; curlun->prevent_medium_removal = 0; if (old_state != FSG_STATE_FORCED_EJECT) { curlun->sense_data = SS_NO_SENSE; curlun->unit_attention_data = SS_NO_SENSE; curlun->sense_data_info = 0; curlun->info_valid = 0; } } } > Also, the sysfs routine should be careful to see whether the command > currently being executed is for the LUN being ejected. I guess you > have never tried issuing your USR1 signal to a mass-storage gadget > running more than one LUN. If you did, you would find that it clears > the prevent_medium_removal flag for all of them, not just the one that > you wanted. I haven't tried it, but I figured it out along the way when I discovered the SIGUSR1 feature. I perceive it as something that should work that way. Like, we hit the whole device. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-07 17:47 ` Maxim Devaev @ 2022-04-08 14:59 ` Alan Stern 2022-04-09 8:57 ` Maxim Devaev 0 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2022-04-08 14:59 UTC (permalink / raw) To: Maxim Devaev Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel On Thu, Apr 07, 2022 at 08:47:13PM +0300, Maxim Devaev wrote: > В Thu, 7 Apr 2022 12:06:01 -0400 > Alan Stern <stern@rowland.harvard.edu> wrote: > > > On Wed, Apr 06, 2022 at 09:36:34PM +0300, Maxim Devaev wrote: > > > В Wed, 6 Apr 2022 13:51:40 -0400 > > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > > On Wed, Apr 06, 2022 at 07:52:34PM +0300, Maxim Devaev wrote: > > > > > > It's not clear to me how breaking I/O operations allows you to do a > > > > > > "force eject". It seems that what you would need is something like > > > > > > fsg_store_file() that omits the curlun->prevent_medium_removal check. > > > > > > Interrupting a lengthy I/O operation doesn't really have anything to do > > > > > > with this. > > > > > > > > > > Perhaps I chose the wrong path, it's just how my userspace code works now. > > > > > If the drive is connected to a Linux host, then in order to clear > > > > > the "file" and extract the image, I sent a SIGUSR1 signal to the "file-storage" > > > > > thread. This interrupted long IO operations, reset curlun->prevent_medium_removal > > > > > and I got the ability to extract. > > > > > > > > Oh, I see. That's kind of an unintended side effect of not calling > > > > raise_exception(). > > > > > > > > And while it does interrupt long I/O operations, it does so in > > > > non-sanctioned way. To the host it will appear as though the gadget's > > > > firmware has crashed, since the gadget will stop sending or receiving > > > > data. Eventually the host will time out and reset the gadget. > > > > > > > > Maybe that's the sort of thing you want, but I rather doubt it. > > > > > > It's hard to say how it actually should work in case of force removing. > > > At least the currect approach with SIGUSR1 is really working on thousands > > > systems and with Linux, Mac and Windows. I believe that the criterion > > > of the experiment is quite important here. I know of several other utilities > > > that use SIGUSR1 for similar purposes. > > > > This merely means that the current unintended behavior of userspace USR1 > > signals must not be changed. But it doesn't mean you have to continue > > to rely on that behavior; you can implement something better. > > So I suggest break_io :) I haven't come up with anything better. But breaking an I/O doesn't do all that you want. That is, interrupting an I/O request (causing an executing command to terminate early) doesn't in itself change the prevent/allow status. Those are two separate operations. The fact that sending a USR1 signal does both is merely a coincidence. Furthermore, it's not clear just what you mean when you say KVM needs to "turn it off immediately". How soon is "immediately"? Even a USR1 signal doesn't work instantaneously. You may find that a forced eject without an I/O interruption works quickly enough. > > > > > Will masking the curlun->prevent_medium_removal flag be enough? > > > > > > > > I think so. But it will be blocked to some extent by long-running I/O > > > > operations, because those operations acquire the filesem rw-semaphore > > > > for reading. > > > > > > > > More precisely, each individual command holds the rw-semaphore. But the > > > > semaphore is dropped between commands, and a long-running I/O operation > > > > typically consists of many separate commands. So the blocking may be > > > > acceptable. > > > > > > It is very important for KVM-over-IP to be able to command "turn it off immediately". > > > > Why is this? A lot of actual devices (DVD drives, for instance) don't > > give you the ability to eject the media when the host has prevented it. > > Why should f-mass-storage be different? > > The DVD drive has the ability to physically eject the disc. You mean by sticking an unfolded paperclip into the manual-eject hole? > It's not too good > for the drive itself, but it's just there. We can also urgently remove > the USB flash drive. Removing a USB flash drive is not a media-eject operation; it's a disconnect operation. (That is, it removes the entire device, not just the media.) By contrast, taking an SD card out from a USB card reader _is_ an example of a media ejection. But card readers do not claim to support the prevent/allow mechanism. > At least there is one situation where the behavior of f_mass_storage differs > from the behavior of a real drive. What happens when you click on the physical > "eject" button? If the host has prevented ejection, nothing happens. Otherwise the disc gets ejected. > Yes, the OS can block this, but the problem is that we don't have > an "eject" here. What do you mean? Writing an empty string to the sysfs "file" attribute is the virtual analog of pressing the eject button. > If I connect the gadget to the Linux host and don't even mount > the image, Linux won't let me change the image in the "file", since the gadget > will be constantly busy with some IO. What I/O are you referring to? Why would a Linux host want to do constant I/O to an unmounted device? Besides, constant I/O shouldn't prevent you from ejecting or changing the backing storage. The eject or change can take place between I/O requests. > But I believe creating a virtual "eject" button is a separate task that > does not depend on "break_io". Do you mean "eject" or "forced eject"? I agree that a virtual "forced eject" is separate from "break_io", and it's probably a lot closer to what you really want. > > > In this context, I would prefer "break_io" rather than "allow_force_remove". > > > > Okay. But what about the 30-second host timeout I mentioned above? > > Does this actually happen with your approach? It seems like the kind of > > thing you don't want in a "turn it off immediately" situation. (I > > haven't tried doing this myself -- maybe I should.) > > Neither I nor my users noticed any problems related to this. After extracting > the image using SIGUSR1/"file", I can just assign a new "file"image > and everything will work. I will try it for myself and see what happens. > > > > > > You should not call send_sig_info() directly; instead call > > > > > > raise_exception(). It already does the work you need (including some > > > > > > things you left out). > > > > > > > > > > raise_exception() assumes the setting of a new state, and I did not want to do this, > > > > > since the same does not happen when throwing a signal from userspace. > > > > > > > > Userspace isn't supposed to send the USR1 signal, only the INT, TERM, or > > > > KILL signals. USR1 is supposed to be reserved for the driver's internal > > > > use. Unfortunately, AFAIK there's no way to allow the driver to send a > > > > signal to itself without also allowing the signal to be sent by > > > > userspace. :-( > > > > > > It's funny that you actually helped me solve my problem thanks to this undocumented > > > behavior. If it were not for the ability to send a signal, I would not be able to make > > > the necessary code, and my software would always be waiting for the completion of IO. > > > > > > So here I am grateful to you - I didn't have to patch the kernel a few years ago, > > > and now I just want to turn it into a clear feature :) > > > > > > Given the needs of the userspace code, maybe the suggested "break_io" > > > would be the best choice? > > > > It sounds like what you really want is a combination of both "interrupt > > I/O" and "forced eject". > > Indeed. But I didn't want to introduce some complex entities into the "file" attribute > or make magic prefixes for the image name or something. So I suggested > "echo > break_io && echo > file". This will not break the current behavior of the drive. Does the host continue to issue "constant" I/O after the broken command? If so, wouldn't that prevent your forced ejection from happening "immediately"? If not, why not? > > > > And sending the signal _does_ set a new state, whether you intended to > > > > or not. Although in this case, the new state is always the same as the > > > > old state, i.e., FSG_STATE_NORMAL. > > > > > > So I could call raise_exception(fsg->common, FSG_STATE_NORMAL) instead of sending > > > the signal from break_io handler. There will be a slight difference > > > in exception_req_tag and exception_arg, but it does not seem to cause any side effects. > > > Please correct me if I'm wrong. > > > > In fact, the best approach would be to introduce a new state (let's call > > it FSG_STATE_FORCED_EJECT) with priority just above > > FSG_STATE_ABORT_BULK_OUT. You would call raise_exception with > > FSG_STATE_FORCED_EJECT, not FSG_STATE_NORMAL. handle_exceptions() would > > treat this state partially like ABORT_BULK_OUT in that it would avoid > > resetting all the LUN data values and would call send_status_common() if > > a command had been underway. But in addition it would do the forced > > eject. > > Do you mean something like this? > > if (old_state != FSG_STATE_ABORT_BULK_OUT) { > for (i = 0; i < ARRAY_SIZE(common->luns); ++i) { > curlun = common->luns[i]; > if (!curlun) > continue; > curlun->prevent_medium_removal = 0; > if (old_state != FSG_STATE_FORCED_EJECT) { > curlun->sense_data = SS_NO_SENSE; > curlun->unit_attention_data = SS_NO_SENSE; > curlun->sense_data_info = 0; > curlun->info_valid = 0; > } > } > } Sort of. > > Also, the sysfs routine should be careful to see whether the command > > currently being executed is for the LUN being ejected. I guess you > > have never tried issuing your USR1 signal to a mass-storage gadget > > running more than one LUN. If you did, you would find that it clears > > the prevent_medium_removal flag for all of them, not just the one that > > you wanted. > > I haven't tried it, but I figured it out along the way when I discovered > the SIGUSR1 feature. I perceive it as something that should work that way. > Like, we hit the whole device. But that's not how real devices work. If you have a multi-LUN DVD drive, for instance, sticking an unfolded paperclip into one of the manual-eject holes will eject only one of the discs, not all of them. Alan Stern ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-08 14:59 ` Alan Stern @ 2022-04-09 8:57 ` Maxim Devaev 2022-04-09 13:46 ` Alan Stern 0 siblings, 1 reply; 17+ messages in thread From: Maxim Devaev @ 2022-04-09 8:57 UTC (permalink / raw) To: Alan Stern Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel В Fri, 8 Apr 2022 10:59:45 -0400 Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, Apr 07, 2022 at 08:47:13PM +0300, Maxim Devaev wrote: > > В Thu, 7 Apr 2022 12:06:01 -0400 > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > On Wed, Apr 06, 2022 at 09:36:34PM +0300, Maxim Devaev wrote: > > > > В Wed, 6 Apr 2022 13:51:40 -0400 > > > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > > > > On Wed, Apr 06, 2022 at 07:52:34PM +0300, Maxim Devaev wrote: > > > > > > > It's not clear to me how breaking I/O operations allows you to do a > > > > > > > "force eject". It seems that what you would need is something like > > > > > > > fsg_store_file() that omits the curlun->prevent_medium_removal check. > > > > > > > Interrupting a lengthy I/O operation doesn't really have anything to do > > > > > > > with this. > > > > > > > > > > > > Perhaps I chose the wrong path, it's just how my userspace code works now. > > > > > > If the drive is connected to a Linux host, then in order to clear > > > > > > the "file" and extract the image, I sent a SIGUSR1 signal to the "file-storage" > > > > > > thread. This interrupted long IO operations, reset curlun->prevent_medium_removal > > > > > > and I got the ability to extract. > > > > > > > > > > Oh, I see. That's kind of an unintended side effect of not calling > > > > > raise_exception(). > > > > > > > > > > And while it does interrupt long I/O operations, it does so in > > > > > non-sanctioned way. To the host it will appear as though the gadget's > > > > > firmware has crashed, since the gadget will stop sending or receiving > > > > > data. Eventually the host will time out and reset the gadget. > > > > > > > > > > Maybe that's the sort of thing you want, but I rather doubt it. > > > > > > > > It's hard to say how it actually should work in case of force removing. > > > > At least the currect approach with SIGUSR1 is really working on thousands > > > > systems and with Linux, Mac and Windows. I believe that the criterion > > > > of the experiment is quite important here. I know of several other utilities > > > > that use SIGUSR1 for similar purposes. > > > > > > This merely means that the current unintended behavior of userspace USR1 > > > signals must not be changed. But it doesn't mean you have to continue > > > to rely on that behavior; you can implement something better. > > > > So I suggest break_io :) I haven't come up with anything better. > > But breaking an I/O doesn't do all that you want. That is, interrupting an > I/O request (causing an executing command to terminate early) doesn't in > itself change the prevent/allow status. Those are two separate operations. > The fact that sending a USR1 signal does both is merely a coincidence. > > Furthermore, it's not clear just what you mean when you say KVM needs to > "turn it off immediately". How soon is "immediately"? Even a USR1 signal > doesn't work instantaneously. You may find that a forced eject without an > I/O interruption works quickly enough. Yes, you're right. I need to focus on forced eject operation. > > > > > > Will masking the curlun->prevent_medium_removal flag be enough? > > > > > > > > > > I think so. But it will be blocked to some extent by long-running I/O > > > > > operations, because those operations acquire the filesem rw-semaphore > > > > > for reading. > > > > > > > > > > More precisely, each individual command holds the rw-semaphore. But the > > > > > semaphore is dropped between commands, and a long-running I/O operation > > > > > typically consists of many separate commands. So the blocking may be > > > > > acceptable. > > > > > > > > It is very important for KVM-over-IP to be able to command "turn it off immediately". > > > > > > Why is this? A lot of actual devices (DVD drives, for instance) don't > > > give you the ability to eject the media when the host has prevented it. > > > Why should f-mass-storage be different? > > > > The DVD drive has the ability to physically eject the disc. > > You mean by sticking an unfolded paperclip into the manual-eject hole? Yes. > > It's not too good > > for the drive itself, but it's just there. We can also urgently remove > > the USB flash drive. > > Removing a USB flash drive is not a media-eject operation; it's a > disconnect operation. (That is, it removes the entire device, not just the > media.) By contrast, taking an SD card out from a USB card reader _is_ an > example of a media ejection. But card readers do not claim to support the > prevent/allow mechanism. > > > At least there is one situation where the behavior of f_mass_storage differs > > from the behavior of a real drive. What happens when you click on the physical > > "eject" button? > > If the host has prevented ejection, nothing happens. Otherwise the disc > gets ejected. > > > Yes, the OS can block this, but the problem is that we don't have > > an "eject" here. > > What do you mean? Writing an empty string to the sysfs "file" attribute > is the virtual analog of pressing the eject button. But I can't eject the disc event it's not mounted on Linux host. It seems to me it differs from the real drive behavior. > ... I have reflected on the rest of your arguments and changed my mind. I think that "forced_eject" for a specific lun without interrupting operations would really be the best solution. I wrote a simple patch and tested it, everything seems to work. What do you think about something like this? static ssize_t fsg_lun_opts_forced_eject_store(struct config_item *item, const char *page, size_t len) { struct fsg_lun_opts *opts = to_fsg_lun_opts(item); struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent); int ret; opts->lun->prevent_medium_removal = 0; ret = fsg_store_file(opts->lun, &fsg_opts->common->filesem, "", 0); return ret < 0 ? ret : len; } CONFIGFS_ATTR_WO(fsg_lun_opts_, forced_eject); If you find this acceptable, I will test this patch on my users to make sure that its behavior meets our expectations. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-09 8:57 ` Maxim Devaev @ 2022-04-09 13:46 ` Alan Stern 2022-04-09 14:08 ` Maxim Devaev 0 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2022-04-09 13:46 UTC (permalink / raw) To: Maxim Devaev Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel On Sat, Apr 09, 2022 at 11:57:56AM +0300, Maxim Devaev wrote: > В Fri, 8 Apr 2022 10:59:45 -0400 > Alan Stern <stern@rowland.harvard.edu> wrote: > > > At least there is one situation where the behavior of f_mass_storage differs > > > from the behavior of a real drive. What happens when you click on the physical > > > "eject" button? > > > > If the host has prevented ejection, nothing happens. Otherwise the disc > > gets ejected. > > > > > Yes, the OS can block this, but the problem is that we don't have > > > an "eject" here. > > > > What do you mean? Writing an empty string to the sysfs "file" attribute > > is the virtual analog of pressing the eject button. > > But I can't eject the disc event it's not mounted on Linux host. It seems to me > it differs from the real drive behavior. It sounds like either there's a bug or else you're not doing the right thing. Tell me exactly what you do when this fails. > > ... > > I have reflected on the rest of your arguments and changed my mind. > I think that "forced_eject" for a specific lun without interrupting operations would > really be the best solution. I wrote a simple patch and tested it, everything seems > to work. What do you think about something like this? > > > static ssize_t fsg_lun_opts_forced_eject_store(struct config_item *item, > const char *page, size_t len) > { > struct fsg_lun_opts *opts = to_fsg_lun_opts(item); > struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent); > int ret; > > opts->lun->prevent_medium_removal = 0; > ret = fsg_store_file(opts->lun, &fsg_opts->common->filesem, "", 0); > return ret < 0 ? ret : len; > } > > CONFIGFS_ATTR_WO(fsg_lun_opts_, forced_eject); The basic idea is right. But this should not be a CONFIGFS option; it should be an ordinary LUN attribute. For an example, see the definition of file_store() in f_mass_storage.c; your routine should look very similar. > If you find this acceptable, I will test this patch on my users to make sure > that its behavior meets our expectations. Okay. Alan Stern ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-09 13:46 ` Alan Stern @ 2022-04-09 14:08 ` Maxim Devaev 2022-04-09 20:22 ` Alan Stern 0 siblings, 1 reply; 17+ messages in thread From: Maxim Devaev @ 2022-04-09 14:08 UTC (permalink / raw) To: Alan Stern Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel В Sat, 9 Apr 2022 09:46:32 -0400 Alan Stern <stern@rowland.harvard.edu> wrote: > On Sat, Apr 09, 2022 at 11:57:56AM +0300, Maxim Devaev wrote: > > В Fri, 8 Apr 2022 10:59:45 -0400 > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > At least there is one situation where the behavior of f_mass_storage differs > > > > from the behavior of a real drive. What happens when you click on the physical > > > > "eject" button? > > > > > > If the host has prevented ejection, nothing happens. Otherwise the disc > > > gets ejected. > > > > > > > Yes, the OS can block this, but the problem is that we don't have > > > > an "eject" here. > > > > > > What do you mean? Writing an empty string to the sysfs "file" attribute > > > is the virtual analog of pressing the eject button. > > > > But I can't eject the disc event it's not mounted on Linux host. It seems to me > > it differs from the real drive behavior. > > It sounds like either there's a bug or else you're not doing the right > thing. Tell me exactly what you do when this fails. I'm using Raspberry Pi with DWC2. So: - Connect RPi-based gadget to the Linux host. - Set image in the "file" attribute. - Mount gadget's drive on the Linux host. - Umount it. - Try to eject using emptying the "file" attribute. - Get EBUSY error. > > > > ... > > > > I have reflected on the rest of your arguments and changed my mind. > > I think that "forced_eject" for a specific lun without interrupting operations would > > really be the best solution. I wrote a simple patch and tested it, everything seems > > to work. What do you think about something like this? > > > > > > static ssize_t fsg_lun_opts_forced_eject_store(struct config_item *item, > > const char *page, size_t len) > > { > > struct fsg_lun_opts *opts = to_fsg_lun_opts(item); > > struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent); > > int ret; > > > > opts->lun->prevent_medium_removal = 0; > > ret = fsg_store_file(opts->lun, &fsg_opts->common->filesem, "", 0); > > return ret < 0 ? ret : len; > > } > > > > CONFIGFS_ATTR_WO(fsg_lun_opts_, forced_eject); > > The basic idea is right. But this should not be a CONFIGFS option; it > should be an ordinary LUN attribute. For an example, see the definition of > file_store() in f_mass_storage.c; your routine should look very similar. Okay, but where this attribute is located in sysfs? How can I use it? Sorry for the stupid question. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-09 14:08 ` Maxim Devaev @ 2022-04-09 20:22 ` Alan Stern 2022-04-09 22:42 ` Maxim Devaev 0 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2022-04-09 20:22 UTC (permalink / raw) To: Maxim Devaev Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel On Sat, Apr 09, 2022 at 05:08:37PM +0300, Maxim Devaev wrote: > В Sat, 9 Apr 2022 09:46:32 -0400 > Alan Stern <stern@rowland.harvard.edu> wrote: > > > On Sat, Apr 09, 2022 at 11:57:56AM +0300, Maxim Devaev wrote: > > > В Fri, 8 Apr 2022 10:59:45 -0400 > > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > At least there is one situation where the behavior of f_mass_storage differs > > > > > from the behavior of a real drive. What happens when you click on the physical > > > > > "eject" button? > > > > > > > > If the host has prevented ejection, nothing happens. Otherwise the disc > > > > gets ejected. > > > > > > > > > Yes, the OS can block this, but the problem is that we don't have > > > > > an "eject" here. > > > > > > > > What do you mean? Writing an empty string to the sysfs "file" attribute > > > > is the virtual analog of pressing the eject button. > > > > > > But I can't eject the disc event it's not mounted on Linux host. It seems to me > > > it differs from the real drive behavior. > > > > It sounds like either there's a bug or else you're not doing the right > > thing. Tell me exactly what you do when this fails. > > I'm using Raspberry Pi with DWC2. So: > - Connect RPi-based gadget to the Linux host. > - Set image in the "file" attribute. Exactly what is the full pathname you're using for the "file" attribute? > - Mount gadget's drive on the Linux host. > - Umount it. > - Try to eject using emptying the "file" attribute. > - Get EBUSY error. This must mean that some program on the host is keeping the device file open, even though it isn't mounted. (I tried running a similar test on my system and it worked perfectly, with no other programs accessing the device). You might be able to identify which program is accessing the device by running lsof on the host and searching the output for the device name. I also tried sending a USR1 signal to the driver's kernel thread while an image was mounted and being accessed. It did clear the prevent_allow flag, so I could eject the image. But it also caused a 30-second delay on the host, as predicted. Now, maybe you don't care about such delays when you're going to eject the media anyway, but it still seems like a bad thing to do. > > > I have reflected on the rest of your arguments and changed my mind. > > > I think that "forced_eject" for a specific lun without interrupting operations would > > > really be the best solution. I wrote a simple patch and tested it, everything seems > > > to work. What do you think about something like this? > > > > > > > > > static ssize_t fsg_lun_opts_forced_eject_store(struct config_item *item, > > > const char *page, size_t len) > > > { > > > struct fsg_lun_opts *opts = to_fsg_lun_opts(item); > > > struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent); > > > int ret; > > > > > > opts->lun->prevent_medium_removal = 0; > > > ret = fsg_store_file(opts->lun, &fsg_opts->common->filesem, "", 0); > > > return ret < 0 ? ret : len; > > > } > > > > > > CONFIGFS_ATTR_WO(fsg_lun_opts_, forced_eject); > > > > The basic idea is right. But this should not be a CONFIGFS option; it > > should be an ordinary LUN attribute. For an example, see the definition of > > file_store() in f_mass_storage.c; your routine should look very similar. > > Okay, but where this attribute is located in sysfs? How can I use it? Well, it's going to be in different places depending on what UDC driver your gadget uses. On my system I'm using the dummy_udc driver, so the sysfs "file" attribute is located at: /sys/devices/platform/dummy_ucd.0/gadget/lun0/file If instead you're looking at /sys/module/g_mass_storage/parameters/file or in some configfs directory, that's the wrong place. You can eject the media simply by doing (as root): echo >/sys/devices/.../gadget/lun0/file (fill in the "..." appropriately for your system). > Sorry for the stupid question. Not at all. Alan Stern ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-09 20:22 ` Alan Stern @ 2022-04-09 22:42 ` Maxim Devaev 2022-04-10 1:57 ` Alan Stern 0 siblings, 1 reply; 17+ messages in thread From: Maxim Devaev @ 2022-04-09 22:42 UTC (permalink / raw) To: Alan Stern Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel В Sat, 9 Apr 2022 16:22:29 -0400 Alan Stern <stern@rowland.harvard.edu> wrote: > On Sat, Apr 09, 2022 at 05:08:37PM +0300, Maxim Devaev wrote: > > В Sat, 9 Apr 2022 09:46:32 -0400 > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > On Sat, Apr 09, 2022 at 11:57:56AM +0300, Maxim Devaev wrote: > > > > В Fri, 8 Apr 2022 10:59:45 -0400 > > > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > At least there is one situation where the behavior of f_mass_storage differs > > > > > > from the behavior of a real drive. What happens when you click on the physical > > > > > > "eject" button? > > > > > > > > > > If the host has prevented ejection, nothing happens. Otherwise the disc > > > > > gets ejected. > > > > > > > > > > > Yes, the OS can block this, but the problem is that we don't have > > > > > > an "eject" here. > > > > > > > > > > What do you mean? Writing an empty string to the sysfs "file" attribute > > > > > is the virtual analog of pressing the eject button. > > > > > > > > But I can't eject the disc event it's not mounted on Linux host. It seems to me > > > > it differs from the real drive behavior. > > > > > > It sounds like either there's a bug or else you're not doing the right > > > thing. Tell me exactly what you do when this fails. > > > > I'm using Raspberry Pi with DWC2. So: > > - Connect RPi-based gadget to the Linux host. > > - Set image in the "file" attribute. > > Exactly what is the full pathname you're using for the "file" attribute? /sys/kernel/config/usb_gadget/kvmd/functions/mass_storage.usb0/lun.0/file > > - Mount gadget's drive on the Linux host. > > - Umount it. > > - Try to eject using emptying the "file" attribute. > > - Get EBUSY error. > > This must mean that some program on the host is keeping the device file > open, even though it isn't mounted. (I tried running a similar test on > my system and it worked perfectly, with no other programs accessing the > device). You might be able to identify which program is accessing the > device by running lsof on the host and searching the output for the > device name. I've been thinking about it too. I tried lsof but it didn't display anything related with sr0 device. But after execution the "eject" command on the host, I was able to emptify the "file" attribute of the gadget. > I also tried sending a USR1 signal to the driver's kernel thread while > an image was mounted and being accessed. It did clear the prevent_allow > flag, so I could eject the image. But it also caused a 30-second delay > on the host, as predicted. Now, maybe you don't care about such delays > when you're going to eject the media anyway, but it still seems like a > bad thing to do. It looks like the prevent_medium_removal flag switching really works better in this case. > > > > > I have reflected on the rest of your arguments and changed my mind. > > > > I think that "forced_eject" for a specific lun without interrupting operations would > > > > really be the best solution. I wrote a simple patch and tested it, everything seems > > > > to work. What do you think about something like this? > > > > > > > > > > > > static ssize_t fsg_lun_opts_forced_eject_store(struct config_item *item, > > > > const char *page, size_t len) > > > > { > > > > struct fsg_lun_opts *opts = to_fsg_lun_opts(item); > > > > struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent); > > > > int ret; > > > > > > > > opts->lun->prevent_medium_removal = 0; > > > > ret = fsg_store_file(opts->lun, &fsg_opts->common->filesem, "", 0); > > > > return ret < 0 ? ret : len; > > > > } > > > > > > > > CONFIGFS_ATTR_WO(fsg_lun_opts_, forced_eject); > > > > > > The basic idea is right. But this should not be a CONFIGFS option; it > > > should be an ordinary LUN attribute. For an example, see the definition of > > > file_store() in f_mass_storage.c; your routine should look very similar. > > > > Okay, but where this attribute is located in sysfs? How can I use it? > > Well, it's going to be in different places depending on what UDC driver > your gadget uses. On my system I'm using the dummy_udc driver, so the > sysfs "file" attribute is located at: > > /sys/devices/platform/dummy_ucd.0/gadget/lun0/file > > If instead you're looking at > > /sys/module/g_mass_storage/parameters/file > > or in some configfs directory, that's the wrong place. You can eject > the media simply by doing (as root): > > echo >/sys/devices/.../gadget/lun0/file > > (fill in the "..." appropriately for your system). > > > Sorry for the stupid question. > > Not at all. Thanks! Unfortunately I'm using dwc2 driver and it doesn't have any gadget parameters outside of the configfs: [root@pikvm ~]# find /sys -iname lun0 [root@pikvm ~]# find /sys -iname lun.0 /sys/kernel/config/usb_gadget/kvmd/functions/mass_storage.usb0/lun.0 [root@pikvm ~]# So in my local case configfs is only way to place forced_eject :( Could we add both device attrs and configfs file? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-09 22:42 ` Maxim Devaev @ 2022-04-10 1:57 ` Alan Stern 2022-04-10 2:18 ` Maxim Devaev 0 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2022-04-10 1:57 UTC (permalink / raw) To: Maxim Devaev Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel On Sun, Apr 10, 2022 at 01:42:28AM +0300, Maxim Devaev wrote: > В Sat, 9 Apr 2022 16:22:29 -0400 > Alan Stern <stern@rowland.harvard.edu> wrote: > > > I'm using Raspberry Pi with DWC2. So: > > > - Connect RPi-based gadget to the Linux host. > > > - Set image in the "file" attribute. > > > > Exactly what is the full pathname you're using for the "file" attribute? > > /sys/kernel/config/usb_gadget/kvmd/functions/mass_storage.usb0/lun.0/file Yeah, that doesn't seem right at all. You're doing this under KVM, right? Is the gadget driver running in the host OS or the guest OS? And the sysfs file accesses -- are they in the host's filesystem or in the guest's? What happens if you don't use KVM and just load the gadget driver on the physical machine? > > > - Mount gadget's drive on the Linux host. > > > - Umount it. > > > - Try to eject using emptying the "file" attribute. > > > - Get EBUSY error. > > > > This must mean that some program on the host is keeping the device file > > open, even though it isn't mounted. (I tried running a similar test on > > my system and it worked perfectly, with no other programs accessing the > > device). You might be able to identify which program is accessing the > > device by running lsof on the host and searching the output for the > > device name. > > I've been thinking about it too. I tried lsof but it didn't display anything > related with sr0 device. But after execution the "eject" command on the host, > I was able to emptify the "file" attribute of the gadget. The eject command should set the "file" attribute to an empty string all by itself, with no need for you to do anything else. > > I also tried sending a USR1 signal to the driver's kernel thread while > > an image was mounted and being accessed. It did clear the prevent_allow > > flag, so I could eject the image. But it also caused a 30-second delay > > on the host, as predicted. Now, maybe you don't care about such delays > > when you're going to eject the media anyway, but it still seems like a > > bad thing to do. > > It looks like the prevent_medium_removal flag switching really works better in this case. I don't understand that comment. In what case? Works better than what? > > > > > I have reflected on the rest of your arguments and changed my mind. > > > > > I think that "forced_eject" for a specific lun without interrupting operations would > > > > > really be the best solution. I wrote a simple patch and tested it, everything seems > > > > > to work. What do you think about something like this? > > > > > > > > > > > > > > > static ssize_t fsg_lun_opts_forced_eject_store(struct config_item *item, > > > > > const char *page, size_t len) > > > > > { > > > > > struct fsg_lun_opts *opts = to_fsg_lun_opts(item); > > > > > struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent); > > > > > int ret; > > > > > > > > > > opts->lun->prevent_medium_removal = 0; > > > > > ret = fsg_store_file(opts->lun, &fsg_opts->common->filesem, "", 0); > > > > > return ret < 0 ? ret : len; > > > > > } > > > > > > > > > > CONFIGFS_ATTR_WO(fsg_lun_opts_, forced_eject); > > > > > > > > The basic idea is right. But this should not be a CONFIGFS option; it > > > > should be an ordinary LUN attribute. For an example, see the definition of > > > > file_store() in f_mass_storage.c; your routine should look very similar. > > > > > > Okay, but where this attribute is located in sysfs? How can I use it? > > > > Well, it's going to be in different places depending on what UDC driver > > your gadget uses. On my system I'm using the dummy_udc driver, so the > > sysfs "file" attribute is located at: > > > > /sys/devices/platform/dummy_ucd.0/gadget/lun0/file > > > > If instead you're looking at > > > > /sys/module/g_mass_storage/parameters/file > > > > or in some configfs directory, that's the wrong place. You can eject > > the media simply by doing (as root): > > > > echo >/sys/devices/.../gadget/lun0/file > > > > (fill in the "..." appropriately for your system). > > > > > Sorry for the stupid question. > > > > Not at all. > > Thanks! Unfortunately I'm using dwc2 driver and it doesn't have any gadget parameters > outside of the configfs: > > [root@pikvm ~]# find /sys -iname lun0 > [root@pikvm ~]# find /sys -iname lun.0 > /sys/kernel/config/usb_gadget/kvmd/functions/mass_storage.usb0/lun.0 > [root@pikvm ~]# > > So in my local case configfs is only way to place forced_eject :( That can't possibly be right. Again, we may be miscommunicating because of the way you're using KVM. What happens if you set up the gadget using g-mass-storage instead of configfs? For example: modprobe g-mass-storage cdrom=y removable=y ro=y file=... > Could we add both device attrs and configfs file? No. Configfs files are for setting up the gadget in the first place, or changing its configuration while it isn't attached to a host. Device attribute files are for modifying the gadget while it is running. Alan Stern ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-10 1:57 ` Alan Stern @ 2022-04-10 2:18 ` Maxim Devaev 2022-04-10 15:21 ` Alan Stern 0 siblings, 1 reply; 17+ messages in thread From: Maxim Devaev @ 2022-04-10 2:18 UTC (permalink / raw) To: Alan Stern Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel В Sat, 9 Apr 2022 21:57:03 -0400 Alan Stern <stern@rowland.harvard.edu> пишет: > On Sun, Apr 10, 2022 at 01:42:28AM +0300, Maxim Devaev wrote: > > В Sat, 9 Apr 2022 16:22:29 -0400 > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > I'm using Raspberry Pi with DWC2. So: > > > > - Connect RPi-based gadget to the Linux host. > > > > - Set image in the "file" attribute. > > > > > > Exactly what is the full pathname you're using for the "file" attribute? > > > > /sys/kernel/config/usb_gadget/kvmd/functions/mass_storage.usb0/lun.0/file > > Yeah, that doesn't seem right at all. > > You're doing this under KVM, right? Is the gadget driver running in the > host OS or the guest OS? And the sysfs file accesses -- are they in the > host's filesystem or in the guest's? > > What happens if you don't use KVM and just load the gadget driver on the > physical machine? We really have a miscommunication :) Speaking of KVM, I mean KVM-over-IP, a physical device that emulates Keyboard-Video-Mouse. It is made on the Raspberry Pi and is physically connected via USB to another host machine to emulate mass storage, among other things. So, we have two physical devices: with USB host and USB gadget. > > > I also tried sending a USR1 signal to the driver's kernel thread while > > > an image was mounted and being accessed. It did clear the prevent_allow > > > flag, so I could eject the image. But it also caused a 30-second delay > > > on the host, as predicted. Now, maybe you don't care about such delays > > > when you're going to eject the media anyway, but it still seems like a > > > bad thing to do. > > > > It looks like the prevent_medium_removal flag switching really works better in this case. > > I don't understand that comment. In what case? Works better than what? Sorry, better than SIGUSR1. The patch that only sets the prevent_medium_removal=0 and makes the "file" empty. > > > > > > > I have reflected on the rest of your arguments and changed my mind. > > > > > > I think that "forced_eject" for a specific lun without interrupting operations would > > > > > > really be the best solution. I wrote a simple patch and tested it, everything seems > > > > > > to work. What do you think about something like this? > > > > > > > > > > > > > > > > > > static ssize_t fsg_lun_opts_forced_eject_store(struct config_item *item, > > > > > > const char *page, size_t len) > > > > > > { > > > > > > struct fsg_lun_opts *opts = to_fsg_lun_opts(item); > > > > > > struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent); > > > > > > int ret; > > > > > > > > > > > > opts->lun->prevent_medium_removal = 0; > > > > > > ret = fsg_store_file(opts->lun, &fsg_opts->common->filesem, "", 0); > > > > > > return ret < 0 ? ret : len; > > > > > > } > > > > > > > > > > > > CONFIGFS_ATTR_WO(fsg_lun_opts_, forced_eject); > > > > > > > > > > The basic idea is right. But this should not be a CONFIGFS option; it > > > > > should be an ordinary LUN attribute. For an example, see the definition of > > > > > file_store() in f_mass_storage.c; your routine should look very similar. > > > > > > > > Okay, but where this attribute is located in sysfs? How can I use it? > > > > > > Well, it's going to be in different places depending on what UDC driver > > > your gadget uses. On my system I'm using the dummy_udc driver, so the > > > sysfs "file" attribute is located at: > > > > > > /sys/devices/platform/dummy_ucd.0/gadget/lun0/file > > > > > > If instead you're looking at > > > > > > /sys/module/g_mass_storage/parameters/file > > > > > > or in some configfs directory, that's the wrong place. You can eject > > > the media simply by doing (as root): > > > > > > echo >/sys/devices/.../gadget/lun0/file > > > > > > (fill in the "..." appropriately for your system). > > > > > > > Sorry for the stupid question. > > > > > > Not at all. > > > > Thanks! Unfortunately I'm using dwc2 driver and it doesn't have any gadget parameters > > outside of the configfs: > > > > [root@pikvm ~]# find /sys -iname lun0 > > [root@pikvm ~]# find /sys -iname lun.0 > > /sys/kernel/config/usb_gadget/kvmd/functions/mass_storage.usb0/lun.0 > > [root@pikvm ~]# > > > > So in my local case configfs is only way to place forced_eject :( > > That can't possibly be right. Again, we may be miscommunicating because > of the way you're using KVM. > > What happens if you set up the gadget using g-mass-storage instead of > configfs? For example: > > modprobe g-mass-storage cdrom=y removable=y ro=y file=... > > > Could we add both device attrs and configfs file? > > No. Configfs files are for setting up the gadget in the first place, or > changing its configuration while it isn't attached to a host. Device > attribute files are for modifying the gadget while it is running. > I've tried and got this: [root@pikvm ~]# modprobe g-mass-storage cdrom=y removable=y ro=y file=/var/lib/kvmd/msd/images/dsl-4.11.rc1.iso [root@pikvm ~]# find /sys -iname lun.0 [root@pikvm ~]# find /sys -iname lun0 /sys/devices/platform/soc/fe980000.usb/gadget/lun0 [root@pikvm ~]# ls /sys/devices/platform/soc/fe980000.usb/gadget/lun0 power file nofua ro uevent But with libcomposite and configfs I don't have "/sys/devices/platform/soc/fe980000.usb/gadget/lun0" at all: [root@pikvm ~]# ls /sys/devices/platform/soc/fe980000.usb/gadget/ power suspended uevent So all this timed I used configfs to change parameters. I thought this was the way it was intended because the code for changing configfs and device attributes is almost identical and everything worked. If I don't have device attributes when using libcomposite, then how am I supposed to change its settings in runtime, if not through configfs? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-10 2:18 ` Maxim Devaev @ 2022-04-10 15:21 ` Alan Stern 2022-04-10 16:14 ` Maxim Devaev 0 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2022-04-10 15:21 UTC (permalink / raw) To: Maxim Devaev Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel On Sun, Apr 10, 2022 at 05:18:04AM +0300, Maxim Devaev wrote: > В Sat, 9 Apr 2022 21:57:03 -0400 > Alan Stern <stern@rowland.harvard.edu> пишет: > > > On Sun, Apr 10, 2022 at 01:42:28AM +0300, Maxim Devaev wrote: > > > В Sat, 9 Apr 2022 16:22:29 -0400 > > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > I'm using Raspberry Pi with DWC2. So: > > > > > - Connect RPi-based gadget to the Linux host. > > > > > - Set image in the "file" attribute. > > > > > > > > Exactly what is the full pathname you're using for the "file" attribute? > > > > > > /sys/kernel/config/usb_gadget/kvmd/functions/mass_storage.usb0/lun.0/file > > > > Yeah, that doesn't seem right at all. > > > > You're doing this under KVM, right? Is the gadget driver running in the > > host OS or the guest OS? And the sysfs file accesses -- are they in the > > host's filesystem or in the guest's? > > > > What happens if you don't use KVM and just load the gadget driver on the > > physical machine? > > We really have a miscommunication :) Speaking of KVM, I mean KVM-over-IP, > a physical device that emulates Keyboard-Video-Mouse. It is made on the > Raspberry Pi and is physically connected via USB to another host machine > to emulate mass storage, among other things. So, we have two physical devices: > with USB host and USB gadget. Okay, I see where I misunderstood. Oops. :-) > > > > I also tried sending a USR1 signal to the driver's kernel thread while > > > > an image was mounted and being accessed. It did clear the prevent_allow > > > > flag, so I could eject the image. But it also caused a 30-second delay > > > > on the host, as predicted. Now, maybe you don't care about such delays > > > > when you're going to eject the media anyway, but it still seems like a > > > > bad thing to do. > > > > > > It looks like the prevent_medium_removal flag switching really works better in this case. > > > > I don't understand that comment. In what case? Works better than what? > > Sorry, better than SIGUSR1. The patch that only sets the prevent_medium_removal=0 > and makes the "file" empty. Ah, yes, I agree. > > > > > > > I have reflected on the rest of your arguments and changed my mind. > > > > > > > I think that "forced_eject" for a specific lun without interrupting operations would > > > > > > > really be the best solution. I wrote a simple patch and tested it, everything seems > > > > > > > to work. What do you think about something like this? > > > > > > > > > > > > > > > > > > > > > static ssize_t fsg_lun_opts_forced_eject_store(struct config_item *item, > > > > > > > const char *page, size_t len) > > > > > > > { > > > > > > > struct fsg_lun_opts *opts = to_fsg_lun_opts(item); > > > > > > > struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent); > > > > > > > int ret; > > > > > > > > > > > > > > opts->lun->prevent_medium_removal = 0; > > > > > > > ret = fsg_store_file(opts->lun, &fsg_opts->common->filesem, "", 0); > > > > > > > return ret < 0 ? ret : len; > > > > > > > } > > > > > > > > > > > > > > CONFIGFS_ATTR_WO(fsg_lun_opts_, forced_eject); > > > > > > > > > > > > The basic idea is right. But this should not be a CONFIGFS option; it > > > > > > should be an ordinary LUN attribute. For an example, see the definition of > > > > > > file_store() in f_mass_storage.c; your routine should look very similar. > > > > > > > > > > Okay, but where this attribute is located in sysfs? How can I use it? > > > > > > > > Well, it's going to be in different places depending on what UDC driver > > > > your gadget uses. On my system I'm using the dummy_udc driver, so the > > > > sysfs "file" attribute is located at: > > > > > > > > /sys/devices/platform/dummy_ucd.0/gadget/lun0/file > > > > > > > > If instead you're looking at > > > > > > > > /sys/module/g_mass_storage/parameters/file > > > > > > > > or in some configfs directory, that's the wrong place. You can eject > > > > the media simply by doing (as root): > > > > > > > > echo >/sys/devices/.../gadget/lun0/file > > > > > > > > (fill in the "..." appropriately for your system). > > > > > > > > > Sorry for the stupid question. > > > > > > > > Not at all. > > > > > > Thanks! Unfortunately I'm using dwc2 driver and it doesn't have any gadget parameters > > > outside of the configfs: > > > > > > [root@pikvm ~]# find /sys -iname lun0 > > > [root@pikvm ~]# find /sys -iname lun.0 > > > /sys/kernel/config/usb_gadget/kvmd/functions/mass_storage.usb0/lun.0 > > > [root@pikvm ~]# > > > > > > So in my local case configfs is only way to place forced_eject :( > > > > That can't possibly be right. Again, we may be miscommunicating because > > of the way you're using KVM. > > > > What happens if you set up the gadget using g-mass-storage instead of > > configfs? For example: > > > > modprobe g-mass-storage cdrom=y removable=y ro=y file=... > > > > > Could we add both device attrs and configfs file? > > > > No. Configfs files are for setting up the gadget in the first place, or > > changing its configuration while it isn't attached to a host. Device > > attribute files are for modifying the gadget while it is running. > > > I've tried and got this: > > [root@pikvm ~]# modprobe g-mass-storage cdrom=y removable=y ro=y file=/var/lib/kvmd/msd/images/dsl-4.11.rc1.iso > [root@pikvm ~]# find /sys -iname lun.0 > [root@pikvm ~]# find /sys -iname lun0 > /sys/devices/platform/soc/fe980000.usb/gadget/lun0 > [root@pikvm ~]# ls /sys/devices/platform/soc/fe980000.usb/gadget/lun0 > power file nofua ro uevent > > But with libcomposite and configfs I don't have "/sys/devices/platform/soc/fe980000.usb/gadget/lun0" at all: > > [root@pikvm ~]# ls /sys/devices/platform/soc/fe980000.usb/gadget/ > power suspended uevent > > So all this timed I used configfs to change parameters. > I thought this was the way it was intended because the code for changing configfs > and device attributes is almost identical and everything worked. > If I don't have device attributes when using libcomposite, then how am I supposed > to change its settings in runtime, if not through configfs? All right. I've never used configfs before, so my understanding of it was out of date. After reading through the documentation and the code, it's clear now that you're right and there should be both a device attribute and a configfs file. Unlike the fsg_lun_opts_forced_eject_store() example you wrote above, but like the existing fsg_lun_opts_file_store() and file_store() routines, both of your new routines should call a single fsg_store_forced_eject() function in storage_common.c to do the real work. Namely, something like: lun->prevent_medium_removal = 0; return fsg_store_file(lun, filesem, "", 0); That should accomplish what you're looking for, in all possible configurations. Alan Stern ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs 2022-04-10 15:21 ` Alan Stern @ 2022-04-10 16:14 ` Maxim Devaev 0 siblings, 0 replies; 17+ messages in thread From: Maxim Devaev @ 2022-04-10 16:14 UTC (permalink / raw) To: Alan Stern Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Cai Huoqing, linux-kernel В Sun, 10 Apr 2022 11:21:15 -0400 Alan Stern <stern@rowland.harvard.edu> wrote: > On Sun, Apr 10, 2022 at 05:18:04AM +0300, Maxim Devaev wrote: > > В Sat, 9 Apr 2022 21:57:03 -0400 > > Alan Stern <stern@rowland.harvard.edu> пишет: > > > > > On Sun, Apr 10, 2022 at 01:42:28AM +0300, Maxim Devaev wrote: > > > > В Sat, 9 Apr 2022 16:22:29 -0400 > > > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > I'm using Raspberry Pi with DWC2. So: > > > > > > - Connect RPi-based gadget to the Linux host. > > > > > > - Set image in the "file" attribute. > > > > > > > > > > Exactly what is the full pathname you're using for the "file" attribute? > > > > > > > > /sys/kernel/config/usb_gadget/kvmd/functions/mass_storage.usb0/lun.0/file > > > > > > Yeah, that doesn't seem right at all. > > > > > > You're doing this under KVM, right? Is the gadget driver running in the > > > host OS or the guest OS? And the sysfs file accesses -- are they in the > > > host's filesystem or in the guest's? > > > > > > What happens if you don't use KVM and just load the gadget driver on the > > > physical machine? > > > > We really have a miscommunication :) Speaking of KVM, I mean KVM-over-IP, > > a physical device that emulates Keyboard-Video-Mouse. It is made on the > > Raspberry Pi and is physically connected via USB to another host machine > > to emulate mass storage, among other things. So, we have two physical devices: > > with USB host and USB gadget. > > Okay, I see where I misunderstood. Oops. :-) > > > > > > I also tried sending a USR1 signal to the driver's kernel thread while > > > > > an image was mounted and being accessed. It did clear the prevent_allow > > > > > flag, so I could eject the image. But it also caused a 30-second delay > > > > > on the host, as predicted. Now, maybe you don't care about such delays > > > > > when you're going to eject the media anyway, but it still seems like a > > > > > bad thing to do. > > > > > > > > It looks like the prevent_medium_removal flag switching really works better in this case. > > > > > > I don't understand that comment. In what case? Works better than what? > > > > Sorry, better than SIGUSR1. The patch that only sets the prevent_medium_removal=0 > > and makes the "file" empty. > > Ah, yes, I agree. > > > > > > > > > I have reflected on the rest of your arguments and changed my mind. > > > > > > > > I think that "forced_eject" for a specific lun without interrupting operations would > > > > > > > > really be the best solution. I wrote a simple patch and tested it, everything seems > > > > > > > > to work. What do you think about something like this? > > > > > > > > > > > > > > > > > > > > > > > > static ssize_t fsg_lun_opts_forced_eject_store(struct config_item *item, > > > > > > > > const char *page, size_t len) > > > > > > > > { > > > > > > > > struct fsg_lun_opts *opts = to_fsg_lun_opts(item); > > > > > > > > struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent); > > > > > > > > int ret; > > > > > > > > > > > > > > > > opts->lun->prevent_medium_removal = 0; > > > > > > > > ret = fsg_store_file(opts->lun, &fsg_opts->common->filesem, "", 0); > > > > > > > > return ret < 0 ? ret : len; > > > > > > > > } > > > > > > > > > > > > > > > > CONFIGFS_ATTR_WO(fsg_lun_opts_, forced_eject); > > > > > > > > > > > > > > The basic idea is right. But this should not be a CONFIGFS option; it > > > > > > > should be an ordinary LUN attribute. For an example, see the definition of > > > > > > > file_store() in f_mass_storage.c; your routine should look very similar. > > > > > > > > > > > > Okay, but where this attribute is located in sysfs? How can I use it? > > > > > > > > > > Well, it's going to be in different places depending on what UDC driver > > > > > your gadget uses. On my system I'm using the dummy_udc driver, so the > > > > > sysfs "file" attribute is located at: > > > > > > > > > > /sys/devices/platform/dummy_ucd.0/gadget/lun0/file > > > > > > > > > > If instead you're looking at > > > > > > > > > > /sys/module/g_mass_storage/parameters/file > > > > > > > > > > or in some configfs directory, that's the wrong place. You can eject > > > > > the media simply by doing (as root): > > > > > > > > > > echo >/sys/devices/.../gadget/lun0/file > > > > > > > > > > (fill in the "..." appropriately for your system). > > > > > > > > > > > Sorry for the stupid question. > > > > > > > > > > Not at all. > > > > > > > > Thanks! Unfortunately I'm using dwc2 driver and it doesn't have any gadget parameters > > > > outside of the configfs: > > > > > > > > [root@pikvm ~]# find /sys -iname lun0 > > > > [root@pikvm ~]# find /sys -iname lun.0 > > > > /sys/kernel/config/usb_gadget/kvmd/functions/mass_storage.usb0/lun.0 > > > > [root@pikvm ~]# > > > > > > > > So in my local case configfs is only way to place forced_eject :( > > > > > > That can't possibly be right. Again, we may be miscommunicating because > > > of the way you're using KVM. > > > > > > What happens if you set up the gadget using g-mass-storage instead of > > > configfs? For example: > > > > > > modprobe g-mass-storage cdrom=y removable=y ro=y file=... > > > > > > > Could we add both device attrs and configfs file? > > > > > > No. Configfs files are for setting up the gadget in the first place, or > > > changing its configuration while it isn't attached to a host. Device > > > attribute files are for modifying the gadget while it is running. > > > > > I've tried and got this: > > > > [root@pikvm ~]# modprobe g-mass-storage cdrom=y removable=y ro=y file=/var/lib/kvmd/msd/images/dsl-4.11.rc1.iso > > [root@pikvm ~]# find /sys -iname lun.0 > > [root@pikvm ~]# find /sys -iname lun0 > > /sys/devices/platform/soc/fe980000.usb/gadget/lun0 > > [root@pikvm ~]# ls /sys/devices/platform/soc/fe980000.usb/gadget/lun0 > > power file nofua ro uevent > > > > But with libcomposite and configfs I don't have "/sys/devices/platform/soc/fe980000.usb/gadget/lun0" at all: > > > > [root@pikvm ~]# ls /sys/devices/platform/soc/fe980000.usb/gadget/ > > power suspended uevent > > > > So all this timed I used configfs to change parameters. > > I thought this was the way it was intended because the code for changing configfs > > and device attributes is almost identical and everything worked. > > If I don't have device attributes when using libcomposite, then how am I supposed > > to change its settings in runtime, if not through configfs? > > All right. I've never used configfs before, so my understanding of it > was out of date. After reading through the documentation and the code, > it's clear now that you're right and there should be both a device > attribute and a configfs file. > > Unlike the fsg_lun_opts_forced_eject_store() example you wrote above, > but like the existing fsg_lun_opts_file_store() and file_store() > routines, both of your new routines should call a single > fsg_store_forced_eject() function in storage_common.c to do the real > work. Namely, something like: > > lun->prevent_medium_removal = 0; > return fsg_store_file(lun, filesem, "", 0); > > That should accomplish what you're looking for, in all possible > configurations. Great! So, next I will test this patch locally and with my users and submit the v2 version a little later. Thank you for your help! ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-04-10 16:14 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-06 9:24 [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs Maxim Devaev 2022-04-06 15:24 ` Alan Stern 2022-04-06 16:52 ` Maxim Devaev 2022-04-06 17:51 ` Alan Stern 2022-04-06 18:36 ` Maxim Devaev 2022-04-07 16:06 ` Alan Stern 2022-04-07 17:47 ` Maxim Devaev 2022-04-08 14:59 ` Alan Stern 2022-04-09 8:57 ` Maxim Devaev 2022-04-09 13:46 ` Alan Stern 2022-04-09 14:08 ` Maxim Devaev 2022-04-09 20:22 ` Alan Stern 2022-04-09 22:42 ` Maxim Devaev 2022-04-10 1:57 ` Alan Stern 2022-04-10 2:18 ` Maxim Devaev 2022-04-10 15:21 ` Alan Stern 2022-04-10 16:14 ` Maxim Devaev
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.