* Re: [PATCH] usb/gadget/function: introduce Built-in CDROM support
2020-06-10 2:32 ` [PATCH] usb/gadget/function: introduce Built-in CDROM support Macpaul Lin
@ 2020-06-10 4:44 ` Peter Chen
2020-06-10 6:15 ` [PATCH v2] " Macpaul Lin
2020-06-10 6:22 ` [PATCH] " Greg Kroah-Hartman
2 siblings, 0 replies; 6+ messages in thread
From: Peter Chen @ 2020-06-10 4:44 UTC (permalink / raw)
To: Macpaul Lin
Cc: Felipe Balbi, Greg Kroah-Hartman, Matthias Brugger, Alan Stern,
Bart Van Assche, Mediatek WSD Upstream, Macpaul Lin,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-usb@vger.kernel.org,
linux-mediatek@lists.infradead.org, Justin Hsieh, Hakieyin Hsieh
On 20-06-10 10:32:29, Macpaul Lin wrote:
> Introduce Built-In CDROM (BICR) support.
> This feature depends on USB_CONFIGFS_MASS_STORAGE option.
>
> 1. Some settings and new function is introduced for BICR.
> 2. Some work around for adapting Android settings is intorduced as well.
%s/intorduced/introduced
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -315,6 +315,9 @@ struct fsg_common {
> void *private_data;
>
> char inquiry_string[INQUIRY_STRING_LEN];
> +
> + /* For build-in CDROM */
> + u8 bicr;
> };
>
> struct fsg_dev {
> @@ -369,6 +372,10 @@ static void set_bulk_out_req_length(struct fsg_common *common,
> if (rem > 0)
> length += common->bulk_out_maxpacket - rem;
> bh->outreq->length = length;
> +
> + /* some USB 2.0 hardware requires this setting */
> + if (IS_ENABLED(USB_CONFIGFS_BICR))
> + bh->outreq->short_not_ok = 1;
> }
Why not use fsg_common.bicr instead of MACRO?
Peter
>
>
> @@ -527,7 +534,16 @@ static int fsg_setup(struct usb_function *f,
> w_length != 1)
> return -EDOM;
> VDBG(fsg, "get max LUN\n");
> - *(u8 *)req->buf = _fsg_common_get_max_lun(fsg->common);
> + if (IS_ENABLED(USB_CONFIGFS_BICR) && fsg->common->bicr) {
> + /*
> + * When Built-In CDROM is enabled,
> + * we share only one LUN.
> + */
> + *(u8 *)req->buf = 0;
> + } else {
> + *(u8 *)req->buf = _fsg_common_get_max_lun(fsg->common);
> + }
> + INFO(fsg, "get max LUN = %d\n", *(u8 *)req->buf);
>
> /* Respond with data/status */
> req->length = min((u16)1, w_length);
> @@ -1329,7 +1345,7 @@ static int do_start_stop(struct fsg_common *common)
> }
>
> /* Are we allowed to unload the media? */
> - if (curlun->prevent_medium_removal) {
> + if (!curlun->nofua && curlun->prevent_medium_removal) {
> LDBG(curlun, "unload attempt prevented\n");
> curlun->sense_data = SS_MEDIUM_REMOVAL_PREVENTED;
> return -EINVAL;
> @@ -2692,6 +2708,7 @@ int fsg_common_set_cdev(struct fsg_common *common,
> common->ep0 = cdev->gadget->ep0;
> common->ep0req = cdev->req;
> common->cdev = cdev;
> + common->bicr = 0;
>
> us = usb_gstrings_attach(cdev, fsg_strings_array,
> ARRAY_SIZE(fsg_strings));
> @@ -2895,6 +2912,33 @@ static void fsg_common_release(struct fsg_common *common)
> kfree(common);
> }
>
> +#ifdef USB_CONFIGFS_BICR
> +ssize_t fsg_bicr_show(struct fsg_common *common, char *buf)
> +{
> + return sprintf(buf, "%d\n", common->bicr);
> +}
> +
> +ssize_t fsg_bicr_store(struct fsg_common *common, const char *buf, size_t size)
> +{
> + int ret;
> +
> + ret = kstrtou8(buf, 10, &common->bicr);
> + if (ret)
> + return -EINVAL;
> +
> + /* Set Lun[0] is a CDROM when enable bicr.*/
> + if (!strcmp(buf, "1"))
> + common->luns[0]->cdrom = 1;
> + else {
> + common->luns[0]->cdrom = 0;
> + common->luns[0]->blkbits = 0;
> + common->luns[0]->blksize = 0;
> + common->luns[0]->num_sectors = 0;
> + }
> +
> + return size;
> +}
> +#endif
>
> /*-------------------------------------------------------------------------*/
>
> @@ -3463,6 +3507,7 @@ void fsg_config_from_params(struct fsg_config *cfg,
> lun->ro = !!params->ro[i];
> lun->cdrom = !!params->cdrom[i];
> lun->removable = !!params->removable[i];
> + lun->nofua = !!params->nofua[i];
> lun->filename =
> params->file_count > i && params->file[i][0]
> ? params->file[i]
> diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
> index 3b8c4ce2a40a..7097e2ea5cc9 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.h
> +++ b/drivers/usb/gadget/function/f_mass_storage.h
> @@ -140,5 +140,8 @@ void fsg_common_set_inquiry_string(struct fsg_common *common, const char *vn,
> void fsg_config_from_params(struct fsg_config *cfg,
> const struct fsg_module_parameters *params,
> unsigned int fsg_num_buffers);
> -
> +#ifdef CONFIG_USB_CONFIGFS_BICR
> +ssize_t fsg_bicr_show(struct fsg_common *common, char *buf);
> +ssize_t fsg_bicr_store(struct fsg_common *common, const char *buf, size_t size);
> +#endif
> #endif /* USB_F_MASS_STORAGE_H */
> diff --git a/drivers/usb/gadget/function/storage_common.c b/drivers/usb/gadget/function/storage_common.c
> index f7e6c42558eb..8fe96eeddf35 100644
> --- a/drivers/usb/gadget/function/storage_common.c
> +++ b/drivers/usb/gadget/function/storage_common.c
> @@ -441,6 +441,29 @@ ssize_t fsg_store_file(struct fsg_lun *curlun, struct rw_semaphore *filesem,
> return -EBUSY; /* "Door is locked" */
> }
>
> + pr_notice("%s file=%s, count=%d, curlun->cdrom=%d\n",
> + __func__, buf, (int)count, curlun->cdrom);
> +
> + /*
> + * WORKAROUND for Android:
> + * VOLD would clean the file path after switching to bicr.
> + * So when the lun is being a CD-ROM a.k.a. BICR.
> + * Don't clean the file path to empty.
> + */
> + if (curlun->cdrom == 1 && count == 1)
> + return count;
> +
> + /*
> + * WORKAROUND: Should be closed the fsg lun for virtual cd-rom,
> + * when switch to other usb functions.
> + * Use the special keyword "off", because the init can
> + * not parse the char '\n' in rc file and write into the sysfs.
> + */
> + if (count == 3 &&
> + buf[0] == 'o' && buf[1] == 'f' && buf[2] == 'f' &&
> + fsg_lun_is_open(curlun))
> + ((char *) buf)[0] = 0;
> +
> /* Remove a trailing newline */
> if (count > 0 && buf[count-1] == '\n')
> ((char *) buf)[count-1] = 0; /* Ugh! */
> --
> 2.18.0
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] usb/gadget/function: introduce Built-in CDROM support
2020-06-10 2:32 ` [PATCH] usb/gadget/function: introduce Built-in CDROM support Macpaul Lin
2020-06-10 4:44 ` Peter Chen
@ 2020-06-10 6:15 ` Macpaul Lin
2020-06-10 14:31 ` Alan Stern
2020-06-10 6:22 ` [PATCH] " Greg Kroah-Hartman
2 siblings, 1 reply; 6+ messages in thread
From: Macpaul Lin @ 2020-06-10 6:15 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, Matthias Brugger, Alan Stern,
Bart Van Assche, EJ Hsu, Mauro Carvalho Chehab,
Benjamin Herrenschmidt, Peter Chen, linux-usb, linux-kernel,
linux-arm-kernel, linux-mediatek
Cc: Mediatek WSD Upstream, Macpaul Lin, Macpaul Lin, Justin Hsieh,
Hakieyin Hsieh
Introduce Built-In CDROM (BICR) support.
This feature depends on USB_CONFIGFS_MASS_STORAGE option.
1. Some settings and new function is introduced for BICR.
2. Some work around for adapting Android settings is introduced as well.
Signed-off-by: Justin Hsieh <justinhsieh@google.com>
Signed-off-by: Hakieyin Hsieh <hakieyin@gmail.com>
Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
---
Changes for v2:
- Thanks for Peter's review.
- Fix typo in commit message.
- use variable common->bicr instead of IS_ENABLED().
- Fix #ifdef CONFIG_USB_CONFIGFS_BICR.
drivers/usb/gadget/Kconfig | 16 +++++++
drivers/usb/gadget/function/f_mass_storage.c | 49 +++++++++++++++++++-
drivers/usb/gadget/function/f_mass_storage.h | 5 +-
drivers/usb/gadget/function/storage_common.c | 23 +++++++++
4 files changed, 90 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 4dc4d48fe6a6..686ba01bedb5 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -188,6 +188,9 @@ config USB_F_RNDIS
config USB_F_MASS_STORAGE
tristate
+config USB_F_BICR
+ tristate
+
config USB_F_FS
tristate
@@ -357,6 +360,19 @@ config USB_CONFIGFS_MASS_STORAGE
device (in much the same way as the "loop" device driver),
specified as a module parameter or sysfs option.
+config USB_CONFIGFS_BICR
+ bool "Built-In CDROM emulation"
+ depends on USB_CONFIGFS
+ depends on BLOCK
+ depends on USB_CONFIGFS_MASS_STORAGE
+ select USB_F_BICR
+ help
+ The Build-In CDROM Gadget acts as a CDROM emulation disk drive.
+ It is based on kernel option "USB_CONFIGFS_MASS_STORAGE".
+ As its storage repository it can use a regular file or a block
+ device (in much the same way as the "loop" device driver),
+ specified as a module parameter or sysfs option.
+
config USB_CONFIGFS_F_LB_SS
bool "Loopback and sourcesink function (for testing)"
depends on USB_CONFIGFS
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 33c2264a0e35..9de1cd465635 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -315,6 +315,9 @@ struct fsg_common {
void *private_data;
char inquiry_string[INQUIRY_STRING_LEN];
+
+ /* For build-in CDROM */
+ u8 bicr;
};
struct fsg_dev {
@@ -369,6 +372,10 @@ static void set_bulk_out_req_length(struct fsg_common *common,
if (rem > 0)
length += common->bulk_out_maxpacket - rem;
bh->outreq->length = length;
+
+ /* some USB 2.0 hardware requires this setting */
+ if (common->bicr)
+ bh->outreq->short_not_ok = 1;
}
@@ -527,7 +534,16 @@ static int fsg_setup(struct usb_function *f,
w_length != 1)
return -EDOM;
VDBG(fsg, "get max LUN\n");
- *(u8 *)req->buf = _fsg_common_get_max_lun(fsg->common);
+ if (IS_ENABLED(USB_CONFIGFS_BICR) && fsg->common->bicr) {
+ /*
+ * When Built-In CDROM is enabled,
+ * we share only one LUN.
+ */
+ *(u8 *)req->buf = 0;
+ } else {
+ *(u8 *)req->buf = _fsg_common_get_max_lun(fsg->common);
+ }
+ INFO(fsg, "get max LUN = %d\n", *(u8 *)req->buf);
/* Respond with data/status */
req->length = min((u16)1, w_length);
@@ -1329,7 +1345,7 @@ static int do_start_stop(struct fsg_common *common)
}
/* Are we allowed to unload the media? */
- if (curlun->prevent_medium_removal) {
+ if (!curlun->nofua && curlun->prevent_medium_removal) {
LDBG(curlun, "unload attempt prevented\n");
curlun->sense_data = SS_MEDIUM_REMOVAL_PREVENTED;
return -EINVAL;
@@ -2692,6 +2708,7 @@ int fsg_common_set_cdev(struct fsg_common *common,
common->ep0 = cdev->gadget->ep0;
common->ep0req = cdev->req;
common->cdev = cdev;
+ common->bicr = 0;
us = usb_gstrings_attach(cdev, fsg_strings_array,
ARRAY_SIZE(fsg_strings));
@@ -2895,6 +2912,33 @@ static void fsg_common_release(struct fsg_common *common)
kfree(common);
}
+#ifdef CONFIG_USB_CONFIGFS_BICR
+ssize_t fsg_bicr_show(struct fsg_common *common, char *buf)
+{
+ return sprintf(buf, "%d\n", common->bicr);
+}
+
+ssize_t fsg_bicr_store(struct fsg_common *common, const char *buf, size_t size)
+{
+ int ret;
+
+ ret = kstrtou8(buf, 10, &common->bicr);
+ if (ret)
+ return -EINVAL;
+
+ /* Set Lun[0] is a CDROM when enable bicr.*/
+ if (!strcmp(buf, "1"))
+ common->luns[0]->cdrom = 1;
+ else {
+ common->luns[0]->cdrom = 0;
+ common->luns[0]->blkbits = 0;
+ common->luns[0]->blksize = 0;
+ common->luns[0]->num_sectors = 0;
+ }
+
+ return size;
+}
+#endif
/*-------------------------------------------------------------------------*/
@@ -3463,6 +3507,7 @@ void fsg_config_from_params(struct fsg_config *cfg,
lun->ro = !!params->ro[i];
lun->cdrom = !!params->cdrom[i];
lun->removable = !!params->removable[i];
+ lun->nofua = !!params->nofua[i];
lun->filename =
params->file_count > i && params->file[i][0]
? params->file[i]
diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
index 3b8c4ce2a40a..7097e2ea5cc9 100644
--- a/drivers/usb/gadget/function/f_mass_storage.h
+++ b/drivers/usb/gadget/function/f_mass_storage.h
@@ -140,5 +140,8 @@ void fsg_common_set_inquiry_string(struct fsg_common *common, const char *vn,
void fsg_config_from_params(struct fsg_config *cfg,
const struct fsg_module_parameters *params,
unsigned int fsg_num_buffers);
-
+#ifdef CONFIG_USB_CONFIGFS_BICR
+ssize_t fsg_bicr_show(struct fsg_common *common, char *buf);
+ssize_t fsg_bicr_store(struct fsg_common *common, const char *buf, size_t size);
+#endif
#endif /* USB_F_MASS_STORAGE_H */
diff --git a/drivers/usb/gadget/function/storage_common.c b/drivers/usb/gadget/function/storage_common.c
index f7e6c42558eb..8fe96eeddf35 100644
--- a/drivers/usb/gadget/function/storage_common.c
+++ b/drivers/usb/gadget/function/storage_common.c
@@ -441,6 +441,29 @@ ssize_t fsg_store_file(struct fsg_lun *curlun, struct rw_semaphore *filesem,
return -EBUSY; /* "Door is locked" */
}
+ pr_notice("%s file=%s, count=%d, curlun->cdrom=%d\n",
+ __func__, buf, (int)count, curlun->cdrom);
+
+ /*
+ * WORKAROUND for Android:
+ * VOLD would clean the file path after switching to bicr.
+ * So when the lun is being a CD-ROM a.k.a. BICR.
+ * Don't clean the file path to empty.
+ */
+ if (curlun->cdrom == 1 && count == 1)
+ return count;
+
+ /*
+ * WORKAROUND: Should be closed the fsg lun for virtual cd-rom,
+ * when switch to other usb functions.
+ * Use the special keyword "off", because the init can
+ * not parse the char '\n' in rc file and write into the sysfs.
+ */
+ if (count == 3 &&
+ buf[0] == 'o' && buf[1] == 'f' && buf[2] == 'f' &&
+ fsg_lun_is_open(curlun))
+ ((char *) buf)[0] = 0;
+
/* Remove a trailing newline */
if (count > 0 && buf[count-1] == '\n')
((char *) buf)[count-1] = 0; /* Ugh! */
--
2.18.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] usb/gadget/function: introduce Built-in CDROM support
2020-06-10 6:15 ` [PATCH v2] " Macpaul Lin
@ 2020-06-10 14:31 ` Alan Stern
2020-06-12 3:38 ` Macpaul Lin
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2020-06-10 14:31 UTC (permalink / raw)
To: Macpaul Lin
Cc: Felipe Balbi, Greg Kroah-Hartman, Matthias Brugger,
Bart Van Assche, EJ Hsu, Mauro Carvalho Chehab,
Benjamin Herrenschmidt, Peter Chen, linux-usb, linux-kernel,
linux-arm-kernel, linux-mediatek, Mediatek WSD Upstream,
Macpaul Lin, Justin Hsieh, Hakieyin Hsieh
On Wed, Jun 10, 2020 at 02:15:18PM +0800, Macpaul Lin wrote:
> Introduce Built-In CDROM (BICR) support.
> This feature depends on USB_CONFIGFS_MASS_STORAGE option.
>
> 1. Some settings and new function is introduced for BICR.
> 2. Some work around for adapting Android settings is introduced as well.
You're going to have to give a much better explanation of what this
does. For people who don't know what Built-In CDROM support is, what
you wrote is meaningless.
For example, how is BICR support different from the CDROM support
already present in the driver? And what's so special about it that it
needs its own kconfig setting?
> @@ -369,6 +372,10 @@ static void set_bulk_out_req_length(struct fsg_common *common,
> if (rem > 0)
> length += common->bulk_out_maxpacket - rem;
> bh->outreq->length = length;
> +
> + /* some USB 2.0 hardware requires this setting */
> + if (common->bicr)
> + bh->outreq->short_not_ok = 1;
How is this connected with BICR? If some USB 2.0 hardware requires this
setting, shouldn't it always be turned on?
Besides, why does some hardware require this? What goes wrong if
short_not_ok is set to 0? If it causes problems, why didn't we become
aware of them many years ago?
> @@ -527,7 +534,16 @@ static int fsg_setup(struct usb_function *f,
> w_length != 1)
> return -EDOM;
> VDBG(fsg, "get max LUN\n");
> - *(u8 *)req->buf = _fsg_common_get_max_lun(fsg->common);
> + if (IS_ENABLED(USB_CONFIGFS_BICR) && fsg->common->bicr) {
> + /*
> + * When Built-In CDROM is enabled,
> + * we share only one LUN.
> + */
> + *(u8 *)req->buf = 0;
> + } else {
> + *(u8 *)req->buf = _fsg_common_get_max_lun(fsg->common);
> + }
This is a very strange way of enforcing a single-LUN restriction. Why
do it here? A much more logical place would be where cfg->nluns is set
up originally.
> + INFO(fsg, "get max LUN = %d\n", *(u8 *)req->buf);
This debugging line isn't needed.
> /* Respond with data/status */
> req->length = min((u16)1, w_length);
> @@ -1329,7 +1345,7 @@ static int do_start_stop(struct fsg_common *common)
> }
>
> /* Are we allowed to unload the media? */
> - if (curlun->prevent_medium_removal) {
> + if (!curlun->nofua && curlun->prevent_medium_removal) {
How is nofua connected to BICR? Or to prevent_medium_removal?
> LDBG(curlun, "unload attempt prevented\n");
> curlun->sense_data = SS_MEDIUM_REMOVAL_PREVENTED;
> return -EINVAL;
> @@ -2692,6 +2708,7 @@ int fsg_common_set_cdev(struct fsg_common *common,
> common->ep0 = cdev->gadget->ep0;
> common->ep0req = cdev->req;
> common->cdev = cdev;
> + common->bicr = 0;
>
> us = usb_gstrings_attach(cdev, fsg_strings_array,
> ARRAY_SIZE(fsg_strings));
> @@ -2895,6 +2912,33 @@ static void fsg_common_release(struct fsg_common *common)
> kfree(common);
> }
>
> +#ifdef CONFIG_USB_CONFIGFS_BICR
> +ssize_t fsg_bicr_show(struct fsg_common *common, char *buf)
> +{
> + return sprintf(buf, "%d\n", common->bicr);
> +}
> +
> +ssize_t fsg_bicr_store(struct fsg_common *common, const char *buf, size_t size)
> +{
> + int ret;
> +
> + ret = kstrtou8(buf, 10, &common->bicr);
> + if (ret)
> + return -EINVAL;
> +
> + /* Set Lun[0] is a CDROM when enable bicr.*/
> + if (!strcmp(buf, "1"))
> + common->luns[0]->cdrom = 1;
> + else {
> + common->luns[0]->cdrom = 0;
> + common->luns[0]->blkbits = 0;
> + common->luns[0]->blksize = 0;
> + common->luns[0]->num_sectors = 0;
> + }
> +
> + return size;
> +}
Where do these attributes ever get exported to sysfs?
> +#endif
>
> /*-------------------------------------------------------------------------*/
>
> @@ -3463,6 +3507,7 @@ void fsg_config_from_params(struct fsg_config *cfg,
> lun->ro = !!params->ro[i];
> lun->cdrom = !!params->cdrom[i];
> lun->removable = !!params->removable[i];
> + lun->nofua = !!params->nofua[i];
Isn't this set already? If not, it is a bug that has nothing to do with
BICR.
> lun->filename =
> params->file_count > i && params->file[i][0]
> ? params->file[i]
> diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
> index 3b8c4ce2a40a..7097e2ea5cc9 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.h
> +++ b/drivers/usb/gadget/function/f_mass_storage.h
> @@ -140,5 +140,8 @@ void fsg_common_set_inquiry_string(struct fsg_common *common, const char *vn,
> void fsg_config_from_params(struct fsg_config *cfg,
> const struct fsg_module_parameters *params,
> unsigned int fsg_num_buffers);
> -
> +#ifdef CONFIG_USB_CONFIGFS_BICR
> +ssize_t fsg_bicr_show(struct fsg_common *common, char *buf);
> +ssize_t fsg_bicr_store(struct fsg_common *common, const char *buf, size_t size);
> +#endif
> #endif /* USB_F_MASS_STORAGE_H */
> diff --git a/drivers/usb/gadget/function/storage_common.c b/drivers/usb/gadget/function/storage_common.c
> index f7e6c42558eb..8fe96eeddf35 100644
> --- a/drivers/usb/gadget/function/storage_common.c
> +++ b/drivers/usb/gadget/function/storage_common.c
> @@ -441,6 +441,29 @@ ssize_t fsg_store_file(struct fsg_lun *curlun, struct rw_semaphore *filesem,
> return -EBUSY; /* "Door is locked" */
> }
>
> + pr_notice("%s file=%s, count=%d, curlun->cdrom=%d\n",
> + __func__, buf, (int)count, curlun->cdrom);
Another debugging line that shouldn't be present in the final patch.
> +
> + /*
> + * WORKAROUND for Android:
> + * VOLD would clean the file path after switching to bicr.
> + * So when the lun is being a CD-ROM a.k.a. BICR.
> + * Don't clean the file path to empty.
> + */
> + if (curlun->cdrom == 1 && count == 1)
> + return count;
> +
> + /*
> + * WORKAROUND: Should be closed the fsg lun for virtual cd-rom,
> + * when switch to other usb functions.
That is not a grammatical English sentence.
> + * Use the special keyword "off", because the init can
> + * not parse the char '\n' in rc file and write into the sysfs.
> + */
> + if (count == 3 &&
> + buf[0] == 'o' && buf[1] == 'f' && buf[2] == 'f' &&
> + fsg_lun_is_open(curlun))
> + ((char *) buf)[0] = 0;
This seems like another bug fix that has no connection with BICR.
Alan Stern
> +
> /* Remove a trailing newline */
> if (count > 0 && buf[count-1] == '\n')
> ((char *) buf)[count-1] = 0; /* Ugh! */
> --
> 2.18.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] usb/gadget/function: introduce Built-in CDROM support
2020-06-10 14:31 ` Alan Stern
@ 2020-06-12 3:38 ` Macpaul Lin
0 siblings, 0 replies; 6+ messages in thread
From: Macpaul Lin @ 2020-06-12 3:38 UTC (permalink / raw)
To: Alan Stern, Greg Kroah-Hartman
Cc: Felipe Balbi, Matthias Brugger, Bart Van Assche, EJ Hsu,
Mauro Carvalho Chehab, Benjamin Herrenschmidt, Peter Chen,
linux-usb, linux-kernel, linux-arm-kernel, linux-mediatek,
Mediatek WSD Upstream, Macpaul Lin, Justin Hsieh, Hakieyin Hsieh
On Wed, 2020-06-10 at 10:31 -0400, Alan Stern wrote:
> On Wed, Jun 10, 2020 at 02:15:18PM +0800, Macpaul Lin wrote:
> > Introduce Built-In CDROM (BICR) support.
> > This feature depends on USB_CONFIGFS_MASS_STORAGE option.
> >
> > 1. Some settings and new function is introduced for BICR.
> > 2. Some work around for adapting Android settings is introduced as well.
>
> You're going to have to give a much better explanation of what this
> does. For people who don't know what Built-In CDROM support is, what
> you wrote is meaningless.
>
> For example, how is BICR support different from the CDROM support
> already present in the driver? And what's so special about it that it
> needs its own kconfig setting?
>
> > @@ -369,6 +372,10 @@ static void set_bulk_out_req_length(struct fsg_common *common,
> > if (rem > 0)
> > length += common->bulk_out_maxpacket - rem;
> > bh->outreq->length = length;
> > +
> > + /* some USB 2.0 hardware requires this setting */
> > + if (common->bicr)
> > + bh->outreq->short_not_ok = 1;
>
> How is this connected with BICR? If some USB 2.0 hardware requires this
> setting, shouldn't it always be turned on?
>
> Besides, why does some hardware require this? What goes wrong if
> short_not_ok is set to 0? If it causes problems, why didn't we become
> aware of them many years ago?
Thanks for Alan and Greg's suggestion, we will check these issues and
see if a better solution could be work out.
> > @@ -527,7 +534,16 @@ static int fsg_setup(struct usb_function *f,
> > w_length != 1)
> > return -EDOM;
> > VDBG(fsg, "get max LUN\n");
> > - *(u8 *)req->buf = _fsg_common_get_max_lun(fsg->common);
> > + if (IS_ENABLED(USB_CONFIGFS_BICR) && fsg->common->bicr) {
> > + /*
> > + * When Built-In CDROM is enabled,
> > + * we share only one LUN.
> > + */
> > + *(u8 *)req->buf = 0;
> > + } else {
> > + *(u8 *)req->buf = _fsg_common_get_max_lun(fsg->common);
> > + }
>
> This is a very strange way of enforcing a single-LUN restriction. Why
> do it here? A much more logical place would be where cfg->nluns is set
> up originally.
>
> > + INFO(fsg, "get max LUN = %d\n", *(u8 *)req->buf);
>
> This debugging line isn't needed.
>
> > /* Respond with data/status */
> > req->length = min((u16)1, w_length);
> > @@ -1329,7 +1345,7 @@ static int do_start_stop(struct fsg_common *common)
> > }
> >
> > /* Are we allowed to unload the media? */
> > - if (curlun->prevent_medium_removal) {
> > + if (!curlun->nofua && curlun->prevent_medium_removal) {
>
> How is nofua connected to BICR? Or to prevent_medium_removal?
>
> > LDBG(curlun, "unload attempt prevented\n");
> > curlun->sense_data = SS_MEDIUM_REMOVAL_PREVENTED;
> > return -EINVAL;
> > @@ -2692,6 +2708,7 @@ int fsg_common_set_cdev(struct fsg_common *common,
> > common->ep0 = cdev->gadget->ep0;
> > common->ep0req = cdev->req;
> > common->cdev = cdev;
> > + common->bicr = 0;
> >
> > us = usb_gstrings_attach(cdev, fsg_strings_array,
> > ARRAY_SIZE(fsg_strings));
> > @@ -2895,6 +2912,33 @@ static void fsg_common_release(struct fsg_common *common)
> > kfree(common);
> > }
> >
> > +#ifdef CONFIG_USB_CONFIGFS_BICR
> > +ssize_t fsg_bicr_show(struct fsg_common *common, char *buf)
> > +{
> > + return sprintf(buf, "%d\n", common->bicr);
> > +}
> > +
> > +ssize_t fsg_bicr_store(struct fsg_common *common, const char *buf, size_t size)
> > +{
> > + int ret;
> > +
> > + ret = kstrtou8(buf, 10, &common->bicr);
> > + if (ret)
> > + return -EINVAL;
> > +
> > + /* Set Lun[0] is a CDROM when enable bicr.*/
> > + if (!strcmp(buf, "1"))
> > + common->luns[0]->cdrom = 1;
> > + else {
> > + common->luns[0]->cdrom = 0;
> > + common->luns[0]->blkbits = 0;
> > + common->luns[0]->blksize = 0;
> > + common->luns[0]->num_sectors = 0;
> > + }
> > +
> > + return size;
> > +}
>
> Where do these attributes ever get exported to sysfs?
>
> > +#endif
> >
> > /*-------------------------------------------------------------------------*/
> >
> > @@ -3463,6 +3507,7 @@ void fsg_config_from_params(struct fsg_config *cfg,
> > lun->ro = !!params->ro[i];
> > lun->cdrom = !!params->cdrom[i];
> > lun->removable = !!params->removable[i];
> > + lun->nofua = !!params->nofua[i];
>
> Isn't this set already? If not, it is a bug that has nothing to do with
> BICR.
>
> > lun->filename =
> > params->file_count > i && params->file[i][0]
> > ? params->file[i]
> > diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
> > index 3b8c4ce2a40a..7097e2ea5cc9 100644
> > --- a/drivers/usb/gadget/function/f_mass_storage.h
> > +++ b/drivers/usb/gadget/function/f_mass_storage.h
> > @@ -140,5 +140,8 @@ void fsg_common_set_inquiry_string(struct fsg_common *common, const char *vn,
> > void fsg_config_from_params(struct fsg_config *cfg,
> > const struct fsg_module_parameters *params,
> > unsigned int fsg_num_buffers);
> > -
> > +#ifdef CONFIG_USB_CONFIGFS_BICR
> > +ssize_t fsg_bicr_show(struct fsg_common *common, char *buf);
> > +ssize_t fsg_bicr_store(struct fsg_common *common, const char *buf, size_t size);
> > +#endif
> > #endif /* USB_F_MASS_STORAGE_H */
> > diff --git a/drivers/usb/gadget/function/storage_common.c b/drivers/usb/gadget/function/storage_common.c
> > index f7e6c42558eb..8fe96eeddf35 100644
> > --- a/drivers/usb/gadget/function/storage_common.c
> > +++ b/drivers/usb/gadget/function/storage_common.c
> > @@ -441,6 +441,29 @@ ssize_t fsg_store_file(struct fsg_lun *curlun, struct rw_semaphore *filesem,
> > return -EBUSY; /* "Door is locked" */
> > }
> >
> > + pr_notice("%s file=%s, count=%d, curlun->cdrom=%d\n",
> > + __func__, buf, (int)count, curlun->cdrom);
>
> Another debugging line that shouldn't be present in the final patch.
>
> > +
> > + /*
> > + * WORKAROUND for Android:
> > + * VOLD would clean the file path after switching to bicr.
> > + * So when the lun is being a CD-ROM a.k.a. BICR.
> > + * Don't clean the file path to empty.
> > + */
> > + if (curlun->cdrom == 1 && count == 1)
> > + return count;
> > +
> > + /*
> > + * WORKAROUND: Should be closed the fsg lun for virtual cd-rom,
> > + * when switch to other usb functions.
>
> That is not a grammatical English sentence.
>
> > + * Use the special keyword "off", because the init can
> > + * not parse the char '\n' in rc file and write into the sysfs.
> > + */
> > + if (count == 3 &&
> > + buf[0] == 'o' && buf[1] == 'f' && buf[2] == 'f' &&
> > + fsg_lun_is_open(curlun))
> > + ((char *) buf)[0] = 0;
>
> This seems like another bug fix that has no connection with BICR.
>
> Alan Stern
>
> > +
> > /* Remove a trailing newline */
> > if (count > 0 && buf[count-1] == '\n')
> > ((char *) buf)[count-1] = 0; /* Ugh! */
> > --
> > 2.18.0
Thanks!
Macpaul Lin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb/gadget/function: introduce Built-in CDROM support
2020-06-10 2:32 ` [PATCH] usb/gadget/function: introduce Built-in CDROM support Macpaul Lin
2020-06-10 4:44 ` Peter Chen
2020-06-10 6:15 ` [PATCH v2] " Macpaul Lin
@ 2020-06-10 6:22 ` Greg Kroah-Hartman
2 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-10 6:22 UTC (permalink / raw)
To: Macpaul Lin
Cc: Felipe Balbi, Matthias Brugger, Alan Stern, Bart Van Assche,
Mediatek WSD Upstream, Macpaul Lin, linux-kernel,
linux-arm-kernel, linux-usb, linux-mediatek, Justin Hsieh,
Hakieyin Hsieh
On Wed, Jun 10, 2020 at 10:32:29AM +0800, Macpaul Lin wrote:
> Introduce Built-In CDROM (BICR) support.
> This feature depends on USB_CONFIGFS_MASS_STORAGE option.
>
> 1. Some settings and new function is introduced for BICR.
> 2. Some work around for adapting Android settings is intorduced as well.
If you have to list a number of things done in a single patch, you
should break this up into multiple patches, as each patch should only do
one thing. Please make this a patch series.
Also, you added new configuration settings, where are they documented?
>
> Signed-off-by: Justin Hsieh <justinhsieh@google.com>
> Signed-off-by: Hakieyin Hsieh <hakieyin@gmail.com>
> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
> ---
> drivers/usb/gadget/Kconfig | 16 +++++++
> drivers/usb/gadget/function/f_mass_storage.c | 49 +++++++++++++++++++-
> drivers/usb/gadget/function/f_mass_storage.h | 5 +-
> drivers/usb/gadget/function/storage_common.c | 23 +++++++++
> 4 files changed, 90 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 4dc4d48fe6a6..686ba01bedb5 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -188,6 +188,9 @@ config USB_F_RNDIS
> config USB_F_MASS_STORAGE
> tristate
>
> +config USB_F_BICR
> + tristate
> +
> config USB_F_FS
> tristate
>
> @@ -357,6 +360,19 @@ config USB_CONFIGFS_MASS_STORAGE
> device (in much the same way as the "loop" device driver),
> specified as a module parameter or sysfs option.
>
> +config USB_CONFIGFS_BICR
> + bool "Built-In CDROM emulation"
> + depends on USB_CONFIGFS
> + depends on BLOCK
> + depends on USB_CONFIGFS_MASS_STORAGE
> + select USB_F_BICR
> + help
> + The Build-In CDROM Gadget acts as a CDROM emulation disk drive.
> + It is based on kernel option "USB_CONFIGFS_MASS_STORAGE".
> + As its storage repository it can use a regular file or a block
> + device (in much the same way as the "loop" device driver),
> + specified as a module parameter or sysfs option.
> +
> config USB_CONFIGFS_F_LB_SS
> bool "Loopback and sourcesink function (for testing)"
> depends on USB_CONFIGFS
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 33c2264a0e35..9de1cd465635 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -315,6 +315,9 @@ struct fsg_common {
> void *private_data;
>
> char inquiry_string[INQUIRY_STRING_LEN];
> +
> + /* For build-in CDROM */
> + u8 bicr;
> };
>
> struct fsg_dev {
> @@ -369,6 +372,10 @@ static void set_bulk_out_req_length(struct fsg_common *common,
> if (rem > 0)
> length += common->bulk_out_maxpacket - rem;
> bh->outreq->length = length;
> +
> + /* some USB 2.0 hardware requires this setting */
> + if (IS_ENABLED(USB_CONFIGFS_BICR))
> + bh->outreq->short_not_ok = 1;
> }
>
>
> @@ -527,7 +534,16 @@ static int fsg_setup(struct usb_function *f,
> w_length != 1)
> return -EDOM;
> VDBG(fsg, "get max LUN\n");
> - *(u8 *)req->buf = _fsg_common_get_max_lun(fsg->common);
> + if (IS_ENABLED(USB_CONFIGFS_BICR) && fsg->common->bicr) {
> + /*
> + * When Built-In CDROM is enabled,
> + * we share only one LUN.
> + */
> + *(u8 *)req->buf = 0;
> + } else {
> + *(u8 *)req->buf = _fsg_common_get_max_lun(fsg->common);
> + }
> + INFO(fsg, "get max LUN = %d\n", *(u8 *)req->buf);
Why this message all the time? Drivers should be quiet if all is good.
>
> /* Respond with data/status */
> req->length = min((u16)1, w_length);
> @@ -1329,7 +1345,7 @@ static int do_start_stop(struct fsg_common *common)
> }
>
> /* Are we allowed to unload the media? */
> - if (curlun->prevent_medium_removal) {
> + if (!curlun->nofua && curlun->prevent_medium_removal) {
> LDBG(curlun, "unload attempt prevented\n");
> curlun->sense_data = SS_MEDIUM_REMOVAL_PREVENTED;
> return -EINVAL;
> @@ -2692,6 +2708,7 @@ int fsg_common_set_cdev(struct fsg_common *common,
> common->ep0 = cdev->gadget->ep0;
> common->ep0req = cdev->req;
> common->cdev = cdev;
> + common->bicr = 0;
>
> us = usb_gstrings_attach(cdev, fsg_strings_array,
> ARRAY_SIZE(fsg_strings));
> @@ -2895,6 +2912,33 @@ static void fsg_common_release(struct fsg_common *common)
> kfree(common);
> }
>
> +#ifdef USB_CONFIGFS_BICR
> +ssize_t fsg_bicr_show(struct fsg_common *common, char *buf)
> +{
> + return sprintf(buf, "%d\n", common->bicr);
> +}
> +
> +ssize_t fsg_bicr_store(struct fsg_common *common, const char *buf, size_t size)
> +{
> + int ret;
> +
> + ret = kstrtou8(buf, 10, &common->bicr);
> + if (ret)
> + return -EINVAL;
> +
> + /* Set Lun[0] is a CDROM when enable bicr.*/
> + if (!strcmp(buf, "1"))
> + common->luns[0]->cdrom = 1;
> + else {
> + common->luns[0]->cdrom = 0;
> + common->luns[0]->blkbits = 0;
> + common->luns[0]->blksize = 0;
> + common->luns[0]->num_sectors = 0;
> + }
> +
> + return size;
> +}
> +#endif
>
> /*-------------------------------------------------------------------------*/
>
> @@ -3463,6 +3507,7 @@ void fsg_config_from_params(struct fsg_config *cfg,
> lun->ro = !!params->ro[i];
> lun->cdrom = !!params->cdrom[i];
> lun->removable = !!params->removable[i];
> + lun->nofua = !!params->nofua[i];
> lun->filename =
> params->file_count > i && params->file[i][0]
> ? params->file[i]
> diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
> index 3b8c4ce2a40a..7097e2ea5cc9 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.h
> +++ b/drivers/usb/gadget/function/f_mass_storage.h
> @@ -140,5 +140,8 @@ void fsg_common_set_inquiry_string(struct fsg_common *common, const char *vn,
> void fsg_config_from_params(struct fsg_config *cfg,
> const struct fsg_module_parameters *params,
> unsigned int fsg_num_buffers);
> -
> +#ifdef CONFIG_USB_CONFIGFS_BICR
> +ssize_t fsg_bicr_show(struct fsg_common *common, char *buf);
> +ssize_t fsg_bicr_store(struct fsg_common *common, const char *buf, size_t size);
> +#endif
> #endif /* USB_F_MASS_STORAGE_H */
> diff --git a/drivers/usb/gadget/function/storage_common.c b/drivers/usb/gadget/function/storage_common.c
> index f7e6c42558eb..8fe96eeddf35 100644
> --- a/drivers/usb/gadget/function/storage_common.c
> +++ b/drivers/usb/gadget/function/storage_common.c
> @@ -441,6 +441,29 @@ ssize_t fsg_store_file(struct fsg_lun *curlun, struct rw_semaphore *filesem,
> return -EBUSY; /* "Door is locked" */
> }
>
> + pr_notice("%s file=%s, count=%d, curlun->cdrom=%d\n",
> + __func__, buf, (int)count, curlun->cdrom);
> +
> + /*
> + * WORKAROUND for Android:
> + * VOLD would clean the file path after switching to bicr.
> + * So when the lun is being a CD-ROM a.k.a. BICR.
> + * Don't clean the file path to empty.
> + */
> + if (curlun->cdrom == 1 && count == 1)
> + return count;
> +
> + /*
> + * WORKAROUND: Should be closed the fsg lun for virtual cd-rom,
> + * when switch to other usb functions.
> + * Use the special keyword "off", because the init can
> + * not parse the char '\n' in rc file and write into the sysfs.
> + */
> + if (count == 3 &&
> + buf[0] == 'o' && buf[1] == 'f' && buf[2] == 'f' &&
> + fsg_lun_is_open(curlun))
> + ((char *) buf)[0] = 0;
Why not fix Android userspace? Wouldn't that be easier?
And the indentation here is really odd...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread