All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/6] usb: gadget: ffs: remove ENTER() macro
@ 2023-03-27 10:12 Linyu Yuan
  2023-03-27 10:12 ` [PATCH v3 2/6] usb: gadget: f_fs: add more function with struct ffs_data *ffs parameter Linyu Yuan
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Linyu Yuan @ 2023-03-27 10:12 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb, Linyu Yuan

ENTER() used to show function name which called during runtime, ftrace can
be used to get same information, let's remove it.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v3: fix build issue Reported-by: kernel test robot <lkp@intel.com>
v2: split to several changes according to v1 comments
v1: https://lore.kernel.org/linux-usb/1679481369-30094-1-git-send-email-quic_linyyuan@quicinc.com/

 drivers/usb/gadget/function/f_fs.c | 94 --------------------------------------
 drivers/usb/gadget/function/u_fs.h |  2 -
 drivers/usb/gadget/legacy/g_ffs.c  |  9 ----
 3 files changed, 105 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index a277c70..8830847 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -335,8 +335,6 @@ static ssize_t ffs_ep0_write(struct file *file, const char __user *buf,
 	ssize_t ret;
 	char *data;
 
-	ENTER();
-
 	/* Fast check if setup was canceled */
 	if (ffs_setup_state_clear_cancelled(ffs) == FFS_SETUP_CANCELLED)
 		return -EIDRM;
@@ -512,8 +510,6 @@ static ssize_t ffs_ep0_read(struct file *file, char __user *buf,
 	size_t n;
 	int ret;
 
-	ENTER();
-
 	/* Fast check if setup was canceled */
 	if (ffs_setup_state_clear_cancelled(ffs) == FFS_SETUP_CANCELLED)
 		return -EIDRM;
@@ -612,8 +608,6 @@ static int ffs_ep0_open(struct inode *inode, struct file *file)
 {
 	struct ffs_data *ffs = inode->i_private;
 
-	ENTER();
-
 	if (ffs->state == FFS_CLOSING)
 		return -EBUSY;
 
@@ -627,8 +621,6 @@ static int ffs_ep0_release(struct inode *inode, struct file *file)
 {
 	struct ffs_data *ffs = file->private_data;
 
-	ENTER();
-
 	ffs_data_closed(ffs);
 
 	return 0;
@@ -640,8 +632,6 @@ static long ffs_ep0_ioctl(struct file *file, unsigned code, unsigned long value)
 	struct usb_gadget *gadget = ffs->gadget;
 	long ret;
 
-	ENTER();
-
 	if (code == FUNCTIONFS_INTERFACE_REVMAP) {
 		struct ffs_function *func = ffs->func;
 		ret = func ? ffs_func_revmap_intf(func, value) : -ENODEV;
@@ -715,7 +705,6 @@ static void ffs_epfile_io_complete(struct usb_ep *_ep, struct usb_request *req)
 {
 	struct ffs_io_data *io_data = req->context;
 
-	ENTER();
 	if (req->status)
 		io_data->status = req->status;
 	else
@@ -856,8 +845,6 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep,
 	struct ffs_io_data *io_data = req->context;
 	struct ffs_data *ffs = io_data->ffs;
 
-	ENTER();
-
 	io_data->status = req->status ? req->status : req->actual;
 	usb_ep_free_request(_ep, req);
 
@@ -1161,8 +1148,6 @@ ffs_epfile_open(struct inode *inode, struct file *file)
 {
 	struct ffs_epfile *epfile = inode->i_private;
 
-	ENTER();
-
 	if (WARN_ON(epfile->ffs->state != FFS_ACTIVE))
 		return -ENODEV;
 
@@ -1179,8 +1164,6 @@ static int ffs_aio_cancel(struct kiocb *kiocb)
 	unsigned long flags;
 	int value;
 
-	ENTER();
-
 	spin_lock_irqsave(&epfile->ffs->eps_lock, flags);
 
 	if (io_data && io_data->ep && io_data->req)
@@ -1198,8 +1181,6 @@ static ssize_t ffs_epfile_write_iter(struct kiocb *kiocb, struct iov_iter *from)
 	struct ffs_io_data io_data, *p = &io_data;
 	ssize_t res;
 
-	ENTER();
-
 	if (!is_sync_kiocb(kiocb)) {
 		p = kzalloc(sizeof(io_data), GFP_KERNEL);
 		if (!p)
@@ -1235,8 +1216,6 @@ static ssize_t ffs_epfile_read_iter(struct kiocb *kiocb, struct iov_iter *to)
 	struct ffs_io_data io_data, *p = &io_data;
 	ssize_t res;
 
-	ENTER();
-
 	if (!is_sync_kiocb(kiocb)) {
 		p = kzalloc(sizeof(io_data), GFP_KERNEL);
 		if (!p)
@@ -1284,8 +1263,6 @@ ffs_epfile_release(struct inode *inode, struct file *file)
 {
 	struct ffs_epfile *epfile = inode->i_private;
 
-	ENTER();
-
 	__ffs_epfile_read_buffer_free(epfile);
 	ffs_data_closed(epfile->ffs);
 
@@ -1299,8 +1276,6 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
 	struct ffs_ep *ep;
 	int ret;
 
-	ENTER();
-
 	if (WARN_ON(epfile->ffs->state != FFS_ACTIVE))
 		return -ENODEV;
 
@@ -1399,8 +1374,6 @@ ffs_sb_make_inode(struct super_block *sb, void *data,
 {
 	struct inode *inode;
 
-	ENTER();
-
 	inode = new_inode(sb);
 
 	if (inode) {
@@ -1432,8 +1405,6 @@ static struct dentry *ffs_sb_create_file(struct super_block *sb,
 	struct dentry	*dentry;
 	struct inode	*inode;
 
-	ENTER();
-
 	dentry = d_alloc_name(sb->s_root, name);
 	if (!dentry)
 		return NULL;
@@ -1468,8 +1439,6 @@ static int ffs_sb_fill(struct super_block *sb, struct fs_context *fc)
 	struct inode	*inode;
 	struct ffs_data	*ffs = data->ffs_data;
 
-	ENTER();
-
 	ffs->sb              = sb;
 	data->ffs_data       = NULL;
 	sb->s_fs_info        = ffs;
@@ -1521,8 +1490,6 @@ static int ffs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	struct fs_parse_result result;
 	int opt;
 
-	ENTER();
-
 	opt = fs_parse(fc, ffs_fs_fs_parameters, param, &result);
 	if (opt < 0)
 		return opt;
@@ -1572,8 +1539,6 @@ static int ffs_fs_get_tree(struct fs_context *fc)
 	struct ffs_data	*ffs;
 	int ret;
 
-	ENTER();
-
 	if (!fc->source)
 		return invalf(fc, "No source specified");
 
@@ -1640,8 +1605,6 @@ static int ffs_fs_init_fs_context(struct fs_context *fc)
 static void
 ffs_fs_kill_sb(struct super_block *sb)
 {
-	ENTER();
-
 	kill_litter_super(sb);
 	if (sb->s_fs_info)
 		ffs_data_closed(sb->s_fs_info);
@@ -1663,8 +1626,6 @@ static int functionfs_init(void)
 {
 	int ret;
 
-	ENTER();
-
 	ret = register_filesystem(&ffs_fs_type);
 	if (!ret)
 		pr_info("file system registered\n");
@@ -1676,8 +1637,6 @@ static int functionfs_init(void)
 
 static void functionfs_cleanup(void)
 {
-	ENTER();
-
 	pr_info("unloading\n");
 	unregister_filesystem(&ffs_fs_type);
 }
@@ -1690,15 +1649,11 @@ static void ffs_data_reset(struct ffs_data *ffs);
 
 static void ffs_data_get(struct ffs_data *ffs)
 {
-	ENTER();
-
 	refcount_inc(&ffs->ref);
 }
 
 static void ffs_data_opened(struct ffs_data *ffs)
 {
-	ENTER();
-
 	refcount_inc(&ffs->ref);
 	if (atomic_add_return(1, &ffs->opened) == 1 &&
 			ffs->state == FFS_DEACTIVATED) {
@@ -1709,8 +1664,6 @@ static void ffs_data_opened(struct ffs_data *ffs)
 
 static void ffs_data_put(struct ffs_data *ffs)
 {
-	ENTER();
-
 	if (refcount_dec_and_test(&ffs->ref)) {
 		pr_info("%s(): freeing\n", __func__);
 		ffs_data_clear(ffs);
@@ -1729,8 +1682,6 @@ static void ffs_data_closed(struct ffs_data *ffs)
 	struct ffs_epfile *epfiles;
 	unsigned long flags;
 
-	ENTER();
-
 	if (atomic_dec_and_test(&ffs->opened)) {
 		if (ffs->no_disconnect) {
 			ffs->state = FFS_DEACTIVATED;
@@ -1765,8 +1716,6 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
 	if (!ffs)
 		return NULL;
 
-	ENTER();
-
 	ffs->io_completion_wq = alloc_ordered_workqueue("%s", 0, dev_name);
 	if (!ffs->io_completion_wq) {
 		kfree(ffs);
@@ -1793,8 +1742,6 @@ static void ffs_data_clear(struct ffs_data *ffs)
 	struct ffs_epfile *epfiles;
 	unsigned long flags;
 
-	ENTER();
-
 	ffs_closed(ffs);
 
 	BUG_ON(ffs->gadget);
@@ -1826,8 +1773,6 @@ static void ffs_data_clear(struct ffs_data *ffs)
 
 static void ffs_data_reset(struct ffs_data *ffs)
 {
-	ENTER();
-
 	ffs_data_clear(ffs);
 
 	ffs->raw_descs_data = NULL;
@@ -1861,8 +1806,6 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
 	struct usb_gadget_strings **lang;
 	int first_id;
 
-	ENTER();
-
 	if (WARN_ON(ffs->state != FFS_ACTIVE
 		 || test_and_set_bit(FFS_FL_BOUND, &ffs->flags)))
 		return -EBADFD;
@@ -1894,8 +1837,6 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
 
 static void functionfs_unbind(struct ffs_data *ffs)
 {
-	ENTER();
-
 	if (!WARN_ON(!ffs->gadget)) {
 		/* dequeue before freeing ep0req */
 		usb_ep_dequeue(ffs->gadget->ep0, ffs->ep0req);
@@ -1914,8 +1855,6 @@ static int ffs_epfiles_create(struct ffs_data *ffs)
 	struct ffs_epfile *epfile, *epfiles;
 	unsigned i, count;
 
-	ENTER();
-
 	count = ffs->eps_count;
 	epfiles = kcalloc(count, sizeof(*epfiles), GFP_KERNEL);
 	if (!epfiles)
@@ -1946,8 +1885,6 @@ static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)
 {
 	struct ffs_epfile *epfile = epfiles;
 
-	ENTER();
-
 	for (; count; --count, ++epfile) {
 		BUG_ON(mutex_is_locked(&epfile->mutex));
 		if (epfile->dentry) {
@@ -2064,8 +2001,6 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
 	u8 length;
 	int ret;
 
-	ENTER();
-
 	/* At least two bytes are required: length and type */
 	if (len < 2) {
 		pr_vdebug("descriptor too short\n");
@@ -2204,8 +2139,6 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
 	unsigned long num = 0;
 	int current_class = -1;
 
-	ENTER();
-
 	for (;;) {
 		int ret;
 
@@ -2243,8 +2176,6 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
 	struct ffs_desc_helper *helper = priv;
 	struct usb_endpoint_descriptor *d;
 
-	ENTER();
-
 	switch (type) {
 	case FFS_DESCRIPTOR:
 		break;
@@ -2329,8 +2260,6 @@ static int __must_check ffs_do_single_os_desc(char *data, unsigned len,
 	int ret;
 	const unsigned _len = len;
 
-	ENTER();
-
 	/* loop over all ext compat/ext prop descriptors */
 	while (feature_count--) {
 		ret = entity(type, h, data, len, priv);
@@ -2352,8 +2281,6 @@ static int __must_check ffs_do_os_descs(unsigned count,
 	const unsigned _len = len;
 	unsigned long num = 0;
 
-	ENTER();
-
 	for (num = 0; num < count; ++num) {
 		int ret;
 		enum ffs_os_desc_type type;
@@ -2416,8 +2343,6 @@ static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
 	struct ffs_data *ffs = priv;
 	u8 length;
 
-	ENTER();
-
 	switch (type) {
 	case FFS_OS_DESC_EXT_COMPAT: {
 		struct usb_ext_compat_desc *d = data;
@@ -2493,8 +2418,6 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 	int ret = -EINVAL, i;
 	struct ffs_desc_helper helper;
 
-	ENTER();
-
 	if (get_unaligned_le32(data + 4) != len)
 		goto error;
 
@@ -2625,8 +2548,6 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
 	const char *data = _data;
 	struct usb_string *s;
 
-	ENTER();
-
 	if (len < 16 ||
 	    get_unaligned_le32(data) != FUNCTIONFS_STRINGS_MAGIC ||
 	    get_unaligned_le32(data + 4) != len)
@@ -3085,8 +3006,6 @@ static inline struct f_fs_opts *ffs_do_functionfs_bind(struct usb_function *f,
 	struct ffs_data *ffs_data;
 	int ret;
 
-	ENTER();
-
 	/*
 	 * Legacy gadget triggers binding in functionfs_ready_callback,
 	 * which already uses locking; taking the same lock here would
@@ -3163,8 +3082,6 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	vla_item_with_sz(d, char, raw_descs, ffs->raw_descs_length);
 	char *vlabuf;
 
-	ENTER();
-
 	/* Has descriptors only for speeds gadget does not support */
 	if (!(full | high | super))
 		return -ENOTSUPP;
@@ -3368,8 +3285,6 @@ static int ffs_func_setup(struct usb_function *f,
 	unsigned long flags;
 	int ret;
 
-	ENTER();
-
 	pr_vdebug("creq->bRequestType = %02x\n", creq->bRequestType);
 	pr_vdebug("creq->bRequest     = %02x\n", creq->bRequest);
 	pr_vdebug("creq->wValue       = %04x\n", le16_to_cpu(creq->wValue));
@@ -3444,13 +3359,11 @@ static bool ffs_func_req_match(struct usb_function *f,
 
 static void ffs_func_suspend(struct usb_function *f)
 {
-	ENTER();
 	ffs_event_add(ffs_func_from_usb(f)->ffs, FUNCTIONFS_SUSPEND);
 }
 
 static void ffs_func_resume(struct usb_function *f)
 {
-	ENTER();
 	ffs_event_add(ffs_func_from_usb(f)->ffs, FUNCTIONFS_RESUME);
 }
 
@@ -3614,7 +3527,6 @@ static void ffs_func_unbind(struct usb_configuration *c,
 	unsigned count = ffs->eps_count;
 	unsigned long flags;
 
-	ENTER();
 	if (ffs->func == func) {
 		ffs_func_eps_disable(func);
 		ffs->func = NULL;
@@ -3654,8 +3566,6 @@ static struct usb_function *ffs_alloc(struct usb_function_instance *fi)
 {
 	struct ffs_function *func;
 
-	ENTER();
-
 	func = kzalloc(sizeof(*func), GFP_KERNEL);
 	if (!func)
 		return ERR_PTR(-ENOMEM);
@@ -3756,7 +3666,6 @@ static int ffs_acquire_dev(const char *dev_name, struct ffs_data *ffs_data)
 	int ret = 0;
 	struct ffs_dev *ffs_dev;
 
-	ENTER();
 	ffs_dev_lock();
 
 	ffs_dev = _ffs_find_dev(dev_name);
@@ -3779,7 +3688,6 @@ static int ffs_acquire_dev(const char *dev_name, struct ffs_data *ffs_data)
 
 static void ffs_release_dev(struct ffs_dev *ffs_dev)
 {
-	ENTER();
 	ffs_dev_lock();
 
 	if (ffs_dev && ffs_dev->mounted) {
@@ -3801,7 +3709,6 @@ static int ffs_ready(struct ffs_data *ffs)
 	struct ffs_dev *ffs_obj;
 	int ret = 0;
 
-	ENTER();
 	ffs_dev_lock();
 
 	ffs_obj = ffs->private_data;
@@ -3834,7 +3741,6 @@ static void ffs_closed(struct ffs_data *ffs)
 	struct f_fs_opts *opts;
 	struct config_item *ci;
 
-	ENTER();
 	ffs_dev_lock();
 
 	ffs_obj = ffs->private_data;
diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
index f102ec2..4b3365f 100644
--- a/drivers/usb/gadget/function/u_fs.h
+++ b/drivers/usb/gadget/function/u_fs.h
@@ -32,8 +32,6 @@
 #  define ffs_dump_mem(prefix, ptr, len) do { } while (0)
 #endif /* VERBOSE_DEBUG */
 
-#define ENTER()    pr_vdebug("%s()\n", __func__)
-
 struct f_fs_opts;
 
 struct ffs_dev {
diff --git a/drivers/usb/gadget/legacy/g_ffs.c b/drivers/usb/gadget/legacy/g_ffs.c
index ae6d8f7..0978546 100644
--- a/drivers/usb/gadget/legacy/g_ffs.c
+++ b/drivers/usb/gadget/legacy/g_ffs.c
@@ -180,8 +180,6 @@ static int __init gfs_init(void)
 	int i;
 	int ret = 0;
 
-	ENTER();
-
 	if (func_num < 2) {
 		gfs_single_func = true;
 		func_num = 1;
@@ -242,8 +240,6 @@ static void __exit gfs_exit(void)
 {
 	int i;
 
-	ENTER();
-
 	if (gfs_registered)
 		usb_composite_unregister(&gfs_driver);
 	gfs_registered = false;
@@ -316,8 +312,6 @@ static int gfs_bind(struct usb_composite_dev *cdev)
 #endif
 	int ret, i;
 
-	ENTER();
-
 	if (missing_funcs)
 		return -ENODEV;
 #if defined CONFIG_USB_FUNCTIONFS_ETH
@@ -445,9 +439,6 @@ static int gfs_unbind(struct usb_composite_dev *cdev)
 {
 	int i;
 
-	ENTER();
-
-
 #ifdef CONFIG_USB_FUNCTIONFS_RNDIS
 	usb_put_function(f_rndis);
 	usb_put_function_instance(fi_rndis);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 2/6] usb: gadget: f_fs: add more function with struct ffs_data *ffs parameter
  2023-03-27 10:12 [PATCH v3 1/6] usb: gadget: ffs: remove ENTER() macro Linyu Yuan
@ 2023-03-27 10:12 ` Linyu Yuan
  2023-03-27 10:12 ` [PATCH v3 3/6] usb: gadget: f_fs: remove struct ffs_data *ffs from struct ffs_desc_helper Linyu Yuan
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 21+ messages in thread
From: Linyu Yuan @ 2023-03-27 10:12 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb, Linyu Yuan

In order to show ffs->dev_name in debug message which is more readable
when multiple f_fs instance exist, it need to add struct ffs_data *ffs
parameter for several functions.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v3: fix review comment, move struct ffs_desc_helper change to single patch
v2: split to several changes according to v1 comments
v1: https://lore.kernel.org/linux-usb/1679481369-30094-1-git-send-email-quic_linyyuan@quicinc.com/

 drivers/usb/gadget/function/f_fs.c | 66 +++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 8830847..32e66f01 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -260,7 +260,7 @@ static void ffs_closed(struct ffs_data *ffs);
 
 static int ffs_mutex_lock(struct mutex *mutex, unsigned nonblock)
 	__attribute__((warn_unused_result, nonnull));
-static char *ffs_prepare_buffer(const char __user *buf, size_t len)
+static char *ffs_prepare_buffer(struct ffs_data *ffs, const char __user *buf, size_t len)
 	__attribute__((warn_unused_result, nonnull));
 
 
@@ -354,7 +354,7 @@ static ssize_t ffs_ep0_write(struct file *file, const char __user *buf,
 			break;
 		}
 
-		data = ffs_prepare_buffer(buf, len);
+		data = ffs_prepare_buffer(ffs, buf, len);
 		if (IS_ERR(data)) {
 			ret = PTR_ERR(data);
 			break;
@@ -426,7 +426,7 @@ static ssize_t ffs_ep0_write(struct file *file, const char __user *buf,
 
 		spin_unlock_irq(&ffs->ev.waitq.lock);
 
-		data = ffs_prepare_buffer(buf, len);
+		data = ffs_prepare_buffer(ffs, buf, len);
 		if (IS_ERR(data)) {
 			ret = PTR_ERR(data);
 			break;
@@ -713,7 +713,8 @@ static void ffs_epfile_io_complete(struct usb_ep *_ep, struct usb_request *req)
 	complete(&io_data->done);
 }
 
-static ssize_t ffs_copy_to_iter(void *data, int data_len, struct iov_iter *iter)
+static ssize_t ffs_copy_to_iter(struct ffs_data *ffs, void *data, int data_len,
+				struct iov_iter *iter)
 {
 	ssize_t ret = copy_to_iter(data, data_len, iter);
 	if (ret == data_len)
@@ -824,7 +825,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
 
 	if (io_data->read && ret > 0) {
 		kthread_use_mm(io_data->mm);
-		ret = ffs_copy_to_iter(io_data->buf, ret, &io_data->data);
+		ret = ffs_copy_to_iter(io_data->ffs, io_data->buf, ret, &io_data->data);
 		kthread_unuse_mm(io_data->mm);
 	}
 
@@ -1984,16 +1985,16 @@ enum ffs_os_desc_type {
 	FFS_OS_DESC, FFS_OS_DESC_EXT_COMPAT, FFS_OS_DESC_EXT_PROP
 };
 
-typedef int (*ffs_entity_callback)(enum ffs_entity_type entity,
+typedef int (*ffs_entity_callback)(struct ffs_data *ffs, enum ffs_entity_type entity,
 				   u8 *valuep,
 				   struct usb_descriptor_header *desc,
 				   void *priv);
 
-typedef int (*ffs_os_desc_callback)(enum ffs_os_desc_type entity,
+typedef int (*ffs_os_desc_callback)(struct ffs_data *ffs, enum ffs_os_desc_type entity,
 				    struct usb_os_desc_header *h, void *data,
 				    unsigned len, void *priv);
 
-static int __must_check ffs_do_single_desc(char *data, unsigned len,
+static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, unsigned len,
 					   ffs_entity_callback entity,
 					   void *priv, int *current_class)
 {
@@ -2023,7 +2024,7 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
 			pr_vdebug("invalid entity's value\n");		\
 			return -EINVAL;					\
 		}							\
-		ret = entity(FFS_ ##type, &val, _ds, priv);		\
+		ret = entity(ffs, FFS_ ##type, &val, _ds, priv);		\
 		if (ret < 0) {						\
 			pr_debug("entity " #type "(%02x); ret = %d\n",	\
 				 (val), ret);				\
@@ -2132,7 +2133,7 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
 	return length;
 }
 
-static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
+static int __must_check ffs_do_descs(struct ffs_data *ffs, unsigned count, char *data, unsigned len,
 				     ffs_entity_callback entity, void *priv)
 {
 	const unsigned _len = len;
@@ -2146,7 +2147,7 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
 			data = NULL;
 
 		/* Record "descriptor" entity */
-		ret = entity(FFS_DESCRIPTOR, (u8 *)num, (void *)data, priv);
+		ret = entity(ffs, FFS_DESCRIPTOR, (u8 *)num, (void *)data, priv);
 		if (ret < 0) {
 			pr_debug("entity DESCRIPTOR(%02lx); ret = %d\n",
 				 num, ret);
@@ -2156,7 +2157,7 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
 		if (!data)
 			return _len - len;
 
-		ret = ffs_do_single_desc(data, len, entity, priv,
+		ret = ffs_do_single_desc(ffs, data, len, entity, priv,
 			&current_class);
 		if (ret < 0) {
 			pr_debug("%s returns %d\n", __func__, ret);
@@ -2169,7 +2170,7 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
 	}
 }
 
-static int __ffs_data_do_entity(enum ffs_entity_type type,
+static int __ffs_data_do_entity(struct ffs_data *ffs, enum ffs_entity_type type,
 				u8 *valuep, struct usb_descriptor_header *desc,
 				void *priv)
 {
@@ -2217,7 +2218,7 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
 	return 0;
 }
 
-static int __ffs_do_os_desc_header(enum ffs_os_desc_type *next_type,
+static int __ffs_do_os_desc_header(struct ffs_data *ffs, enum ffs_os_desc_type *next_type,
 				   struct usb_os_desc_header *desc)
 {
 	u16 bcd_version = le16_to_cpu(desc->bcdVersion);
@@ -2250,7 +2251,7 @@ static int __ffs_do_os_desc_header(enum ffs_os_desc_type *next_type,
  * Process all extended compatibility/extended property descriptors
  * of a feature descriptor
  */
-static int __must_check ffs_do_single_os_desc(char *data, unsigned len,
+static int __must_check ffs_do_single_os_desc(struct ffs_data *ffs, char *data, unsigned len,
 					      enum ffs_os_desc_type type,
 					      u16 feature_count,
 					      ffs_os_desc_callback entity,
@@ -2262,7 +2263,7 @@ static int __must_check ffs_do_single_os_desc(char *data, unsigned len,
 
 	/* loop over all ext compat/ext prop descriptors */
 	while (feature_count--) {
-		ret = entity(type, h, data, len, priv);
+		ret = entity(ffs, type, h, data, len, priv);
 		if (ret < 0) {
 			pr_debug("bad OS descriptor, type: %d\n", type);
 			return ret;
@@ -2274,7 +2275,7 @@ static int __must_check ffs_do_single_os_desc(char *data, unsigned len,
 }
 
 /* Process a number of complete Feature Descriptors (Ext Compat or Ext Prop) */
-static int __must_check ffs_do_os_descs(unsigned count,
+static int __must_check ffs_do_os_descs(struct ffs_data *ffs, unsigned count,
 					char *data, unsigned len,
 					ffs_os_desc_callback entity, void *priv)
 {
@@ -2300,7 +2301,7 @@ static int __must_check ffs_do_os_descs(unsigned count,
 		if (le32_to_cpu(desc->dwLength) > len)
 			return -EINVAL;
 
-		ret = __ffs_do_os_desc_header(&type, desc);
+		ret = __ffs_do_os_desc_header(ffs, &type, desc);
 		if (ret < 0) {
 			pr_debug("entity OS_DESCRIPTOR(%02lx); ret = %d\n",
 				 num, ret);
@@ -2320,7 +2321,7 @@ static int __must_check ffs_do_os_descs(unsigned count,
 		 * Process all function/property descriptors
 		 * of this Feature Descriptor
 		 */
-		ret = ffs_do_single_os_desc(data, len, type,
+		ret = ffs_do_single_os_desc(ffs, data, len, type,
 					    feature_count, entity, priv, desc);
 		if (ret < 0) {
 			pr_debug("%s returns %d\n", __func__, ret);
@@ -2336,11 +2337,10 @@ static int __must_check ffs_do_os_descs(unsigned count,
 /*
  * Validate contents of the buffer from userspace related to OS descriptors.
  */
-static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
+static int __ffs_data_do_os_desc(struct ffs_data *ffs, enum ffs_os_desc_type type,
 				 struct usb_os_desc_header *h, void *data,
 				 unsigned len, void *priv)
 {
-	struct ffs_data *ffs = priv;
 	u8 length;
 
 	switch (type) {
@@ -2491,7 +2491,7 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 			continue;
 		helper.interfaces_count = 0;
 		helper.eps_count = 0;
-		ret = ffs_do_descs(counts[i], data, len,
+		ret = ffs_do_descs(ffs, counts[i], data, len,
 				   __ffs_data_do_entity, &helper);
 		if (ret < 0)
 			goto error;
@@ -2512,8 +2512,8 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 		len  -= ret;
 	}
 	if (os_descs_count) {
-		ret = ffs_do_os_descs(os_descs_count, data, len,
-				      __ffs_data_do_os_desc, ffs);
+		ret = ffs_do_os_descs(ffs, os_descs_count, data, len,
+				      __ffs_data_do_os_desc, NULL);
 		if (ret < 0)
 			goto error;
 		data += ret;
@@ -2763,7 +2763,7 @@ static int ffs_ep_addr2idx(struct ffs_data *ffs, u8 endpoint_address)
 	return -ENOENT;
 }
 
-static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
+static int __ffs_func_bind_do_descs(struct ffs_data *ffs, enum ffs_entity_type type, u8 *valuep,
 				    struct usb_descriptor_header *desc,
 				    void *priv)
 {
@@ -2863,7 +2863,7 @@ static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
 	return 0;
 }
 
-static int __ffs_func_bind_do_nums(enum ffs_entity_type type, u8 *valuep,
+static int __ffs_func_bind_do_nums(struct ffs_data *ffs, enum ffs_entity_type type, u8 *valuep,
 				   struct usb_descriptor_header *desc,
 				   void *priv)
 {
@@ -2918,7 +2918,7 @@ static int __ffs_func_bind_do_nums(enum ffs_entity_type type, u8 *valuep,
 	return 0;
 }
 
-static int __ffs_func_bind_do_os_desc(enum ffs_os_desc_type type,
+static int __ffs_func_bind_do_os_desc(struct ffs_data *ffs, enum ffs_os_desc_type type,
 				      struct usb_os_desc_header *h, void *data,
 				      unsigned len, void *priv)
 {
@@ -3119,7 +3119,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	 */
 	if (full) {
 		func->function.fs_descriptors = vla_ptr(vlabuf, d, fs_descs);
-		fs_len = ffs_do_descs(ffs->fs_descs_count,
+		fs_len = ffs_do_descs(ffs, ffs->fs_descs_count,
 				      vla_ptr(vlabuf, d, raw_descs),
 				      d_raw_descs__sz,
 				      __ffs_func_bind_do_descs, func);
@@ -3133,7 +3133,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
 
 	if (high) {
 		func->function.hs_descriptors = vla_ptr(vlabuf, d, hs_descs);
-		hs_len = ffs_do_descs(ffs->hs_descs_count,
+		hs_len = ffs_do_descs(ffs, ffs->hs_descs_count,
 				      vla_ptr(vlabuf, d, raw_descs) + fs_len,
 				      d_raw_descs__sz - fs_len,
 				      __ffs_func_bind_do_descs, func);
@@ -3148,7 +3148,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	if (super) {
 		func->function.ss_descriptors = func->function.ssp_descriptors =
 			vla_ptr(vlabuf, d, ss_descs);
-		ss_len = ffs_do_descs(ffs->ss_descs_count,
+		ss_len = ffs_do_descs(ffs, ffs->ss_descs_count,
 				vla_ptr(vlabuf, d, raw_descs) + fs_len + hs_len,
 				d_raw_descs__sz - fs_len - hs_len,
 				__ffs_func_bind_do_descs, func);
@@ -3165,7 +3165,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	 * endpoint numbers rewriting.  We can do that in one go
 	 * now.
 	 */
-	ret = ffs_do_descs(ffs->fs_descs_count +
+	ret = ffs_do_descs(ffs, ffs->fs_descs_count +
 			   (high ? ffs->hs_descs_count : 0) +
 			   (super ? ffs->ss_descs_count : 0),
 			   vla_ptr(vlabuf, d, raw_descs), d_raw_descs__sz,
@@ -3185,7 +3185,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
 				vla_ptr(vlabuf, d, ext_compat) + i * 16;
 			INIT_LIST_HEAD(&desc->ext_prop);
 		}
-		ret = ffs_do_os_descs(ffs->ms_os_descs_count,
+		ret = ffs_do_os_descs(ffs, ffs->ms_os_descs_count,
 				      vla_ptr(vlabuf, d, raw_descs) +
 				      fs_len + hs_len + ss_len,
 				      d_raw_descs__sz - fs_len - hs_len -
@@ -3781,7 +3781,7 @@ static int ffs_mutex_lock(struct mutex *mutex, unsigned nonblock)
 		: mutex_lock_interruptible(mutex);
 }
 
-static char *ffs_prepare_buffer(const char __user *buf, size_t len)
+static char *ffs_prepare_buffer(struct ffs_data *ffs, const char __user *buf, size_t len)
 {
 	char *data;
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 3/6] usb: gadget: f_fs: remove struct ffs_data *ffs from struct ffs_desc_helper
  2023-03-27 10:12 [PATCH v3 1/6] usb: gadget: ffs: remove ENTER() macro Linyu Yuan
  2023-03-27 10:12 ` [PATCH v3 2/6] usb: gadget: f_fs: add more function with struct ffs_data *ffs parameter Linyu Yuan
@ 2023-03-27 10:12 ` Linyu Yuan
  2023-03-27 10:12 ` [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev Linyu Yuan
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 21+ messages in thread
From: Linyu Yuan @ 2023-03-27 10:12 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb, Linyu Yuan

As struct ffs_desc_helper have one struct ffs_data *ffs member, but it
related function already have struct ffs_data *ffs parameter, it is safe
to remove the struct member.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v3: new patch in this version

 drivers/usb/gadget/function/f_fs.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 32e66f01..a4051c8 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -231,7 +231,6 @@ struct ffs_io_data {
 };
 
 struct ffs_desc_helper {
-	struct ffs_data *ffs;
 	unsigned interfaces_count;
 	unsigned eps_count;
 };
@@ -2196,8 +2195,8 @@ static int __ffs_data_do_entity(struct ffs_data *ffs, enum ffs_entity_type type,
 		 * Strings are indexed from 1 (0 is reserved
 		 * for languages list)
 		 */
-		if (*valuep > helper->ffs->strings_count)
-			helper->ffs->strings_count = *valuep;
+		if (*valuep > ffs->strings_count)
+			ffs->strings_count = *valuep;
 		break;
 
 	case FFS_ENDPOINT:
@@ -2206,10 +2205,10 @@ static int __ffs_data_do_entity(struct ffs_data *ffs, enum ffs_entity_type type,
 		if (helper->eps_count >= FFS_MAX_EPS_COUNT)
 			return -EINVAL;
 		/* Check if descriptors for any speed were already parsed */
-		if (!helper->ffs->eps_count && !helper->ffs->interfaces_count)
-			helper->ffs->eps_addrmap[helper->eps_count] =
+		if (!ffs->eps_count && !ffs->interfaces_count)
+			ffs->eps_addrmap[helper->eps_count] =
 				d->bEndpointAddress;
-		else if (helper->ffs->eps_addrmap[helper->eps_count] !=
+		else if (ffs->eps_addrmap[helper->eps_count] !=
 				d->bEndpointAddress)
 			return -EINVAL;
 		break;
@@ -2485,7 +2484,6 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 
 	/* Read descriptors */
 	raw_descs = data;
-	helper.ffs = ffs;
 	for (i = 0; i < 3; ++i) {
 		if (!counts[i])
 			continue;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev
  2023-03-27 10:12 [PATCH v3 1/6] usb: gadget: ffs: remove ENTER() macro Linyu Yuan
  2023-03-27 10:12 ` [PATCH v3 2/6] usb: gadget: f_fs: add more function with struct ffs_data *ffs parameter Linyu Yuan
  2023-03-27 10:12 ` [PATCH v3 3/6] usb: gadget: f_fs: remove struct ffs_data *ffs from struct ffs_desc_helper Linyu Yuan
@ 2023-03-27 10:12 ` Linyu Yuan
  2023-03-29  6:53   ` Greg Kroah-Hartman
  2023-03-27 10:12 ` [PATCH v3 5/6] usb: gadget: f_fs: replace private pr_vdebug() with dev_vdbg Linyu Yuan
  2023-03-27 10:12 ` [PATCH v3 6/6] usb: gadget: f_fs: show instance name in debug message Linyu Yuan
  4 siblings, 1 reply; 21+ messages in thread
From: Linyu Yuan @ 2023-03-27 10:12 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb, Linyu Yuan

It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer.

Add a struct device *dev member in struct ffs_data, set it to NULL before
binding or after unbinding to a usb_gadget, set it reference of usb_gadget
->dev when bind success.

Then it can help replace private pr_vdebug() to dev_vdbg() consistently.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v3: new patch in this version

 drivers/usb/gadget/function/f_fs.c | 3 +++
 drivers/usb/gadget/function/u_fs.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index a4051c8..25461f1 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
 		return NULL;
 	}
 
+	ffs->dev = NULL;
 	refcount_set(&ffs->ref, 1);
 	atomic_set(&ffs->opened, 0);
 	ffs->state = FFS_READ_DESCRIPTORS;
@@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
 	}
 
 	ffs->gadget = cdev->gadget;
+	ffs->dev = &cdev->gadget->dev;
 	ffs_data_get(ffs);
 	return 0;
 }
@@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs)
 		mutex_lock(&ffs->mutex);
 		usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req);
 		ffs->ep0req = NULL;
+		ffs->dev = NULL;
 		ffs->gadget = NULL;
 		clear_bit(FFS_FL_BOUND, &ffs->flags);
 		mutex_unlock(&ffs->mutex);
diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
index 4b3365f..c5f6167 100644
--- a/drivers/usb/gadget/function/u_fs.h
+++ b/drivers/usb/gadget/function/u_fs.h
@@ -146,6 +146,7 @@ enum ffs_setup_state {
 
 struct ffs_data {
 	struct usb_gadget		*gadget;
+	struct device			*dev;
 
 	/*
 	 * Protect access read/write operations, only one read/write
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 5/6] usb: gadget: f_fs: replace private pr_vdebug() with dev_vdbg
  2023-03-27 10:12 [PATCH v3 1/6] usb: gadget: ffs: remove ENTER() macro Linyu Yuan
                   ` (2 preceding siblings ...)
  2023-03-27 10:12 ` [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev Linyu Yuan
@ 2023-03-27 10:12 ` Linyu Yuan
  2023-03-29  6:55   ` Greg Kroah-Hartman
  2023-03-27 10:12 ` [PATCH v3 6/6] usb: gadget: f_fs: show instance name in debug message Linyu Yuan
  4 siblings, 1 reply; 21+ messages in thread
From: Linyu Yuan @ 2023-03-27 10:12 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb, Linyu Yuan

Use command dev_vdbg() macro to show some debug message.

Also replace some pr_debug/err/warn/info() to dev_dbg/err/warn/info().

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v3: new patch in this version

 drivers/usb/gadget/function/f_fs.c | 98 +++++++++++++++++++-------------------
 drivers/usb/gadget/function/u_fs.h |  6 ---
 2 files changed, 49 insertions(+), 55 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 25461f1..0761eaa 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -317,12 +317,12 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
 static int __ffs_ep0_stall(struct ffs_data *ffs)
 {
 	if (ffs->ev.can_stall) {
-		pr_vdebug("ep0 stall\n");
+		dev_vdbg(ffs->dev, "ep0 stall\n");
 		usb_ep_set_halt(ffs->gadget->ep0);
 		ffs->setup_state = FFS_NO_SETUP;
 		return -EL2HLT;
 	} else {
-		pr_debug("bogus ep0 stall!\n");
+		dev_dbg(ffs->dev, "bogus ep0 stall!\n");
 		return -ESRCH;
 	}
 }
@@ -361,7 +361,7 @@ static ssize_t ffs_ep0_write(struct file *file, const char __user *buf,
 
 		/* Handle data */
 		if (ffs->state == FFS_READ_DESCRIPTORS) {
-			pr_info("read descriptors\n");
+			dev_info(ffs->dev, "read descriptors\n");
 			ret = __ffs_data_got_descs(ffs, data, len);
 			if (ret < 0)
 				break;
@@ -369,7 +369,7 @@ static ssize_t ffs_ep0_write(struct file *file, const char __user *buf,
 			ffs->state = FFS_READ_STRINGS;
 			ret = len;
 		} else {
-			pr_info("read strings\n");
+			dev_info(ffs->dev, "read strings\n");
 			ret = __ffs_data_got_strings(ffs, data, len);
 			if (ret < 0)
 				break;
@@ -749,7 +749,7 @@ static ssize_t ffs_copy_to_iter(struct ffs_data *ffs, void *data, int data_len,
 	 * aio_read(2) etc. system calls.  Writing data to an IN endpoint is not
 	 * affected.
 	 */
-	pr_err("functionfs read size %d > requested size %zd, dropping excess data. "
+	dev_err(ffs->dev, "functionfs read size %d > requested size %zd, dropping excess data. "
 	       "Align read buffer size to max packet size to avoid the problem.\n",
 	       data_len, ret);
 
@@ -911,7 +911,7 @@ static ssize_t __ffs_epfile_read_data(struct ffs_epfile *epfile,
 		return -EFAULT;
 
 	/* See ffs_copy_to_iter for more context. */
-	pr_warn("functionfs read size %d > requested size %zd, splitting request into multiple reads.",
+	dev_warn(epfile->ffs->dev, "functionfs read size %d > requested size %zd, splitting request into multiple reads.",
 		data_len, ret);
 
 	data_len -= ret;
@@ -1665,7 +1665,7 @@ static void ffs_data_opened(struct ffs_data *ffs)
 static void ffs_data_put(struct ffs_data *ffs)
 {
 	if (refcount_dec_and_test(&ffs->ref)) {
-		pr_info("%s(): freeing\n", __func__);
+		dev_info(ffs->dev, "%s(): freeing\n", __func__);
 		ffs_data_clear(ffs);
 		ffs_release_dev(ffs->private_data);
 		BUG_ON(waitqueue_active(&ffs->ev.waitq) ||
@@ -1945,7 +1945,7 @@ static int ffs_func_eps_enable(struct ffs_function *func)
 
 		ret = config_ep_by_speed(func->gadget, &func->function, ep->ep);
 		if (ret) {
-			pr_err("%s: config_ep_by_speed(%s) returned %d\n",
+			dev_err(ffs->dev, "%s: config_ep_by_speed(%s) returned %d\n",
 					__func__, ep->ep->name, ret);
 			break;
 		}
@@ -2006,14 +2006,14 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 
 	/* At least two bytes are required: length and type */
 	if (len < 2) {
-		pr_vdebug("descriptor too short\n");
+		dev_vdbg(ffs->dev, "descriptor too short\n");
 		return -EINVAL;
 	}
 
 	/* If we have at least as many bytes as the descriptor takes? */
 	length = _ds->bLength;
 	if (len < length) {
-		pr_vdebug("descriptor longer then available data\n");
+		dev_vdbg(ffs->dev, "descriptor longer then available data\n");
 		return -EINVAL;
 	}
 
@@ -2021,14 +2021,14 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 #define __entity_check_STRING(val)     (val)
 #define __entity_check_ENDPOINT(val)   ((val) & USB_ENDPOINT_NUMBER_MASK)
 #define __entity(type, val) do {					\
-		pr_vdebug("entity " #type "(%02x)\n", (val));		\
+		dev_vdbg(ffs->dev, "entity " #type "(%02x)\n", (val));		\
 		if (!__entity_check_ ##type(val)) {			\
-			pr_vdebug("invalid entity's value\n");		\
+			dev_vdbg(ffs->dev, "invalid entity's value\n");		\
 			return -EINVAL;					\
 		}							\
 		ret = entity(ffs, FFS_ ##type, &val, _ds, priv);		\
 		if (ret < 0) {						\
-			pr_debug("entity " #type "(%02x); ret = %d\n",	\
+			dev_dbg(ffs->dev, "entity " #type "(%02x); ret = %d\n",	\
 				 (val), ret);				\
 			return ret;					\
 		}							\
@@ -2041,13 +2041,13 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 	case USB_DT_STRING:
 	case USB_DT_DEVICE_QUALIFIER:
 		/* function can't have any of those */
-		pr_vdebug("descriptor reserved for gadget: %d\n",
+		dev_vdbg(ffs->dev, "descriptor reserved for gadget: %d\n",
 		      _ds->bDescriptorType);
 		return -EINVAL;
 
 	case USB_DT_INTERFACE: {
 		struct usb_interface_descriptor *ds = (void *)_ds;
-		pr_vdebug("interface descriptor\n");
+		dev_vdbg(ffs->dev, "interface descriptor\n");
 		if (length != sizeof *ds)
 			goto inv_length;
 
@@ -2060,7 +2060,7 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 
 	case USB_DT_ENDPOINT: {
 		struct usb_endpoint_descriptor *ds = (void *)_ds;
-		pr_vdebug("endpoint descriptor\n");
+		dev_vdbg(ffs->dev, "endpoint descriptor\n");
 		if (length != USB_DT_ENDPOINT_SIZE &&
 		    length != USB_DT_ENDPOINT_AUDIO_SIZE)
 			goto inv_length;
@@ -2070,17 +2070,17 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 
 	case USB_TYPE_CLASS | 0x01:
 		if (*current_class == USB_INTERFACE_CLASS_HID) {
-			pr_vdebug("hid descriptor\n");
+			dev_vdbg(ffs->dev, "hid descriptor\n");
 			if (length != sizeof(struct hid_descriptor))
 				goto inv_length;
 			break;
 		} else if (*current_class == USB_INTERFACE_CLASS_CCID) {
-			pr_vdebug("ccid descriptor\n");
+			dev_vdbg(ffs->dev, "ccid descriptor\n");
 			if (length != sizeof(struct ccid_descriptor))
 				goto inv_length;
 			break;
 		} else {
-			pr_vdebug("unknown descriptor: %d for class %d\n",
+			dev_vdbg(ffs->dev, "unknown descriptor: %d for class %d\n",
 			      _ds->bDescriptorType, *current_class);
 			return -EINVAL;
 		}
@@ -2092,7 +2092,7 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 
 	case USB_DT_INTERFACE_ASSOCIATION: {
 		struct usb_interface_assoc_descriptor *ds = (void *)_ds;
-		pr_vdebug("interface association descriptor\n");
+		dev_vdbg(ffs->dev, "interface association descriptor\n");
 		if (length != sizeof *ds)
 			goto inv_length;
 		if (ds->iFunction)
@@ -2101,7 +2101,7 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 		break;
 
 	case USB_DT_SS_ENDPOINT_COMP:
-		pr_vdebug("EP SS companion descriptor\n");
+		dev_vdbg(ffs->dev, "EP SS companion descriptor\n");
 		if (length != sizeof(struct usb_ss_ep_comp_descriptor))
 			goto inv_length;
 		break;
@@ -2112,16 +2112,16 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 	case USB_DT_SECURITY:
 	case USB_DT_CS_RADIO_CONTROL:
 		/* TODO */
-		pr_vdebug("unimplemented descriptor: %d\n", _ds->bDescriptorType);
+		dev_vdbg(ffs->dev, "unimplemented descriptor: %d\n", _ds->bDescriptorType);
 		return -EINVAL;
 
 	default:
 		/* We should never be here */
-		pr_vdebug("unknown descriptor: %d\n", _ds->bDescriptorType);
+		dev_vdbg(ffs->dev, "unknown descriptor: %d\n", _ds->bDescriptorType);
 		return -EINVAL;
 
 inv_length:
-		pr_vdebug("invalid length: %d (descriptor %d)\n",
+		dev_vdbg(ffs->dev, "invalid length: %d (descriptor %d)\n",
 			  _ds->bLength, _ds->bDescriptorType);
 		return -EINVAL;
 	}
@@ -2151,7 +2151,7 @@ static int __must_check ffs_do_descs(struct ffs_data *ffs, unsigned count, char
 		/* Record "descriptor" entity */
 		ret = entity(ffs, FFS_DESCRIPTOR, (u8 *)num, (void *)data, priv);
 		if (ret < 0) {
-			pr_debug("entity DESCRIPTOR(%02lx); ret = %d\n",
+			dev_dbg(ffs->dev, "entity DESCRIPTOR(%02lx); ret = %d\n",
 				 num, ret);
 			return ret;
 		}
@@ -2162,7 +2162,7 @@ static int __must_check ffs_do_descs(struct ffs_data *ffs, unsigned count, char
 		ret = ffs_do_single_desc(ffs, data, len, entity, priv,
 			&current_class);
 		if (ret < 0) {
-			pr_debug("%s returns %d\n", __func__, ret);
+			dev_dbg(ffs->dev, "%s returns %d\n", __func__, ret);
 			return ret;
 		}
 
@@ -2227,10 +2227,10 @@ static int __ffs_do_os_desc_header(struct ffs_data *ffs, enum ffs_os_desc_type *
 	u16 w_index = le16_to_cpu(desc->wIndex);
 
 	if (bcd_version == 0x1) {
-		pr_warn("bcdVersion must be 0x0100, stored in Little Endian order. "
+		dev_warn(ffs->dev, "bcdVersion must be 0x0100, stored in Little Endian order. "
 			"Userspace driver should be fixed, accepting 0x0001 for compatibility.\n");
 	} else if (bcd_version != 0x100) {
-		pr_vdebug("unsupported os descriptors version: 0x%x\n",
+		dev_vdbg(ffs->dev, "unsupported os descriptors version: 0x%x\n",
 			  bcd_version);
 		return -EINVAL;
 	}
@@ -2242,7 +2242,7 @@ static int __ffs_do_os_desc_header(struct ffs_data *ffs, enum ffs_os_desc_type *
 		*next_type = FFS_OS_DESC_EXT_PROP;
 		break;
 	default:
-		pr_vdebug("unsupported os descriptor type: %d", w_index);
+		dev_vdbg(ffs->dev, "unsupported os descriptor type: %d", w_index);
 		return -EINVAL;
 	}
 
@@ -2267,7 +2267,7 @@ static int __must_check ffs_do_single_os_desc(struct ffs_data *ffs, char *data,
 	while (feature_count--) {
 		ret = entity(ffs, type, h, data, len, priv);
 		if (ret < 0) {
-			pr_debug("bad OS descriptor, type: %d\n", type);
+			dev_dbg(ffs->dev, "bad OS descriptor, type: %d\n", type);
 			return ret;
 		}
 		data += ret;
@@ -2305,7 +2305,7 @@ static int __must_check ffs_do_os_descs(struct ffs_data *ffs, unsigned count,
 
 		ret = __ffs_do_os_desc_header(ffs, &type, desc);
 		if (ret < 0) {
-			pr_debug("entity OS_DESCRIPTOR(%02lx); ret = %d\n",
+			dev_dbg(ffs->dev, "entity OS_DESCRIPTOR(%02lx); ret = %d\n",
 				 num, ret);
 			return ret;
 		}
@@ -2326,7 +2326,7 @@ static int __must_check ffs_do_os_descs(struct ffs_data *ffs, unsigned count,
 		ret = ffs_do_single_os_desc(ffs, data, len, type,
 					    feature_count, entity, priv, desc);
 		if (ret < 0) {
-			pr_debug("%s returns %d\n", __func__, ret);
+			dev_dbg(ffs->dev, "%s returns %d\n", __func__, ret);
 			return ret;
 		}
 
@@ -2360,7 +2360,7 @@ static int __ffs_data_do_os_desc(struct ffs_data *ffs, enum ffs_os_desc_type typ
 			 * values.  We fix it here to avoid returning EINVAL
 			 * in response to values we used to accept.
 			 */
-			pr_debug("usb_ext_compat_desc::Reserved1 forced to 1\n");
+			dev_dbg(ffs->dev, "usb_ext_compat_desc::Reserved1 forced to 1\n");
 			d->Reserved1 = 1;
 		}
 		for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
@@ -2383,19 +2383,19 @@ static int __ffs_data_do_os_desc(struct ffs_data *ffs, enum ffs_os_desc_type typ
 		type = le32_to_cpu(d->dwPropertyDataType);
 		if (type < USB_EXT_PROP_UNICODE ||
 		    type > USB_EXT_PROP_UNICODE_MULTI) {
-			pr_vdebug("unsupported os descriptor property type: %d",
+			dev_vdbg(ffs->dev, "unsupported os descriptor property type: %d",
 				  type);
 			return -EINVAL;
 		}
 		pnl = le16_to_cpu(d->wPropertyNameLength);
 		if (length < 14 + pnl) {
-			pr_vdebug("invalid os descriptor length: %d pnl:%d (descriptor %d)\n",
+			dev_vdbg(ffs->dev, "invalid os descriptor length: %d pnl:%d (descriptor %d)\n",
 				  length, pnl, type);
 			return -EINVAL;
 		}
 		pdl = le32_to_cpu(*(__le32 *)((u8 *)data + 10 + pnl));
 		if (length != 14 + pnl + pdl) {
-			pr_vdebug("invalid os descriptor length: %d pnl:%d pdl:%d (descriptor %d)\n",
+			dev_vdbg(ffs->dev, "invalid os descriptor length: %d pnl:%d pdl:%d (descriptor %d)\n",
 				  length, pnl, pdl, type);
 			return -EINVAL;
 		}
@@ -2406,7 +2406,7 @@ static int __ffs_data_do_os_desc(struct ffs_data *ffs, enum ffs_os_desc_type typ
 	}
 		break;
 	default:
-		pr_vdebug("unknown descriptor: %d\n", type);
+		dev_vdbg(ffs->dev, "unknown descriptor: %d\n", type);
 		return -EINVAL;
 	}
 	return length;
@@ -2732,11 +2732,11 @@ static void __ffs_event_add(struct ffs_data *ffs,
 			if ((*ev == rem_type1 || *ev == rem_type2) == neg)
 				*out++ = *ev;
 			else
-				pr_vdebug("purging event %d\n", *ev);
+				dev_vdbg(ffs->dev, "purging event %d\n", *ev);
 		ffs->ev.count = out - ffs->ev.types;
 	}
 
-	pr_vdebug("adding event %d\n", type);
+	dev_vdbg(ffs->dev, "adding event %d\n", type);
 	ffs->ev.types[ffs->ev.count++] = type;
 	wake_up_locked(&ffs->ev.waitq);
 	if (ffs->ffs_eventfd)
@@ -2805,7 +2805,7 @@ static int __ffs_func_bind_do_descs(struct ffs_data *ffs, enum ffs_entity_type t
 	ffs_ep = func->eps + idx;
 
 	if (ffs_ep->descs[ep_desc_id]) {
-		pr_err("two %sspeed descriptors for EP %d\n",
+		dev_err(ffs->dev, "two %sspeed descriptors for EP %d\n",
 			  speed_names[ep_desc_id],
 			  ds->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK);
 		return -EINVAL;
@@ -2833,7 +2833,7 @@ static int __ffs_func_bind_do_descs(struct ffs_data *ffs, enum ffs_entity_type t
 		 * endpoint descriptors as if they were full speed.
 		 */
 		wMaxPacketSize = ds->wMaxPacketSize;
-		pr_vdebug("autoconfig\n");
+		dev_vdbg(ffs->dev, "autoconfig\n");
 		ep = usb_ep_autoconfig(func->gadget, ds);
 		if (!ep)
 			return -ENOTSUPP;
@@ -2914,7 +2914,7 @@ static int __ffs_func_bind_do_nums(struct ffs_data *ffs, enum ffs_entity_type ty
 		break;
 	}
 
-	pr_vdebug("%02x -> %02x\n", *valuep, newValue);
+	dev_vdbg(ffs->dev, "%02x -> %02x\n", *valuep, newValue);
 	*valuep = newValue;
 	return 0;
 }
@@ -2992,7 +2992,7 @@ static int __ffs_func_bind_do_os_desc(struct ffs_data *ffs, enum ffs_os_desc_typ
 	}
 		break;
 	default:
-		pr_vdebug("unknown descriptor: %d\n", type);
+		dev_vdbg(ffs->dev, "unknown descriptor: %d\n", type);
 	}
 
 	return length;
@@ -3286,11 +3286,11 @@ static int ffs_func_setup(struct usb_function *f,
 	unsigned long flags;
 	int ret;
 
-	pr_vdebug("creq->bRequestType = %02x\n", creq->bRequestType);
-	pr_vdebug("creq->bRequest     = %02x\n", creq->bRequest);
-	pr_vdebug("creq->wValue       = %04x\n", le16_to_cpu(creq->wValue));
-	pr_vdebug("creq->wIndex       = %04x\n", le16_to_cpu(creq->wIndex));
-	pr_vdebug("creq->wLength      = %04x\n", le16_to_cpu(creq->wLength));
+	dev_vdbg(ffs->dev, "creq->bRequestType = %02x\n", creq->bRequestType);
+	dev_vdbg(ffs->dev, "creq->bRequest     = %02x\n", creq->bRequest);
+	dev_vdbg(ffs->dev, "creq->wValue       = %04x\n", le16_to_cpu(creq->wValue));
+	dev_vdbg(ffs->dev, "creq->wIndex       = %04x\n", le16_to_cpu(creq->wIndex));
+	dev_vdbg(ffs->dev, "creq->wLength      = %04x\n", le16_to_cpu(creq->wLength));
 
 	/*
 	 * Most requests directed to interface go through here
@@ -3793,7 +3793,7 @@ static char *ffs_prepare_buffer(struct ffs_data *ffs, const char __user *buf, si
 	if (IS_ERR(data))
 		return data;
 
-	pr_vdebug("Buffer from user space:\n");
+	dev_vdbg(ffs->dev, "Buffer from user space:\n");
 	ffs_dump_mem("", data, len);
 
 	return data;
diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
index c5f6167..1b70bd7 100644
--- a/drivers/usb/gadget/function/u_fs.h
+++ b/drivers/usb/gadget/function/u_fs.h
@@ -20,15 +20,9 @@
 #include <linux/refcount.h>
 
 #ifdef VERBOSE_DEBUG
-#ifndef pr_vdebug
-#  define pr_vdebug pr_debug
-#endif /* pr_vdebug */
 #  define ffs_dump_mem(prefix, ptr, len) \
 	print_hex_dump_bytes(pr_fmt(prefix ": "), DUMP_PREFIX_NONE, ptr, len)
 #else
-#ifndef pr_vdebug
-#  define pr_vdebug(...)                 do { } while (0)
-#endif /* pr_vdebug */
 #  define ffs_dump_mem(prefix, ptr, len) do { } while (0)
 #endif /* VERBOSE_DEBUG */
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 6/6] usb: gadget: f_fs: show instance name in debug message
  2023-03-27 10:12 [PATCH v3 1/6] usb: gadget: ffs: remove ENTER() macro Linyu Yuan
                   ` (3 preceding siblings ...)
  2023-03-27 10:12 ` [PATCH v3 5/6] usb: gadget: f_fs: replace private pr_vdebug() with dev_vdbg Linyu Yuan
@ 2023-03-27 10:12 ` Linyu Yuan
  2023-03-29  6:54   ` Greg Kroah-Hartman
  4 siblings, 1 reply; 21+ messages in thread
From: Linyu Yuan @ 2023-03-27 10:12 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb, Linyu Yuan

show ffs->dev_name in all possible debug message.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v3: change according comments
v2: split to several changes according to v1 comments
v1: https://lore.kernel.org/linux-usb/1679481369-30094-1-git-send-email-quic_linyyuan@quicinc.com/

 drivers/usb/gadget/function/f_fs.c | 141 ++++++++++++++++++++-----------------
 1 file changed, 75 insertions(+), 66 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 0761eaa..383343d 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -317,12 +317,12 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
 static int __ffs_ep0_stall(struct ffs_data *ffs)
 {
 	if (ffs->ev.can_stall) {
-		dev_vdbg(ffs->dev, "ep0 stall\n");
+		dev_vdbg(ffs->dev, "%s: ep0 stall\n", ffs->dev_name);
 		usb_ep_set_halt(ffs->gadget->ep0);
 		ffs->setup_state = FFS_NO_SETUP;
 		return -EL2HLT;
 	} else {
-		dev_dbg(ffs->dev, "bogus ep0 stall!\n");
+		dev_dbg(ffs->dev, "%s: bogus ep0 stall!\n", ffs->dev_name);
 		return -ESRCH;
 	}
 }
@@ -361,7 +361,7 @@ static ssize_t ffs_ep0_write(struct file *file, const char __user *buf,
 
 		/* Handle data */
 		if (ffs->state == FFS_READ_DESCRIPTORS) {
-			dev_info(ffs->dev, "read descriptors\n");
+			dev_info(ffs->dev, "%s: read descriptors\n", ffs->dev_name);
 			ret = __ffs_data_got_descs(ffs, data, len);
 			if (ret < 0)
 				break;
@@ -369,7 +369,7 @@ static ssize_t ffs_ep0_write(struct file *file, const char __user *buf,
 			ffs->state = FFS_READ_STRINGS;
 			ret = len;
 		} else {
-			dev_info(ffs->dev, "read strings\n");
+			dev_info(ffs->dev, "%s: read strings\n", ffs->dev_name);
 			ret = __ffs_data_got_strings(ffs, data, len);
 			if (ret < 0)
 				break;
@@ -749,9 +749,9 @@ static ssize_t ffs_copy_to_iter(struct ffs_data *ffs, void *data, int data_len,
 	 * aio_read(2) etc. system calls.  Writing data to an IN endpoint is not
 	 * affected.
 	 */
-	dev_err(ffs->dev, "functionfs read size %d > requested size %zd, dropping excess data. "
+	dev_err(ffs->dev, "%s: functionfs read size %d > requested size %zd, dropping excess data. "
 	       "Align read buffer size to max packet size to avoid the problem.\n",
-	       data_len, ret);
+	       ffs->dev_name, data_len, ret);
 
 	return ret;
 }
@@ -911,8 +911,8 @@ static ssize_t __ffs_epfile_read_data(struct ffs_epfile *epfile,
 		return -EFAULT;
 
 	/* See ffs_copy_to_iter for more context. */
-	dev_warn(epfile->ffs->dev, "functionfs read size %d > requested size %zd, splitting request into multiple reads.",
-		data_len, ret);
+	dev_warn(epfile->ffs->dev, "%s: functionfs read size %d > requested size %zd, splitting request into multiple reads.",
+		epfile->ffs->dev_name, data_len, ret);
 
 	data_len -= ret;
 	buf = kmalloc(struct_size(buf, storage, data_len), GFP_KERNEL);
@@ -1043,7 +1043,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		 * For such reason, we're adding this redundant sanity check
 		 * here.
 		 */
-		WARN(1, "%s: data_len == -EINVAL\n", __func__);
+		WARN(1, "%s: %s(): data_len == -EINVAL\n", epfile->ffs->dev_name, __func__);
 		ret = -EINVAL;
 	} else if (!io_data->aio) {
 		bool interrupted = false;
@@ -1665,7 +1665,7 @@ static void ffs_data_opened(struct ffs_data *ffs)
 static void ffs_data_put(struct ffs_data *ffs)
 {
 	if (refcount_dec_and_test(&ffs->ref)) {
-		dev_info(ffs->dev, "%s(): freeing\n", __func__);
+		dev_info(ffs->dev, "%s: %s(): freeing\n", ffs->dev_name, __func__);
 		ffs_data_clear(ffs);
 		ffs_release_dev(ffs->private_data);
 		BUG_ON(waitqueue_active(&ffs->ev.waitq) ||
@@ -1945,8 +1945,8 @@ static int ffs_func_eps_enable(struct ffs_function *func)
 
 		ret = config_ep_by_speed(func->gadget, &func->function, ep->ep);
 		if (ret) {
-			dev_err(ffs->dev, "%s: config_ep_by_speed(%s) returned %d\n",
-					__func__, ep->ep->name, ret);
+			dev_err(ffs->dev, "%s: %s(): config_ep_by_speed(%s) returned %d\n",
+					ffs->dev_name, __func__, ep->ep->name, ret);
 			break;
 		}
 
@@ -2006,14 +2006,14 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 
 	/* At least two bytes are required: length and type */
 	if (len < 2) {
-		dev_vdbg(ffs->dev, "descriptor too short\n");
+		dev_vdbg(ffs->dev, "%s: descriptor too short\n", ffs->dev_name);
 		return -EINVAL;
 	}
 
 	/* If we have at least as many bytes as the descriptor takes? */
 	length = _ds->bLength;
 	if (len < length) {
-		dev_vdbg(ffs->dev, "descriptor longer then available data\n");
+		dev_vdbg(ffs->dev, "%s: descriptor longer then available data\n", ffs->dev_name);
 		return -EINVAL;
 	}
 
@@ -2021,15 +2021,15 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 #define __entity_check_STRING(val)     (val)
 #define __entity_check_ENDPOINT(val)   ((val) & USB_ENDPOINT_NUMBER_MASK)
 #define __entity(type, val) do {					\
-		dev_vdbg(ffs->dev, "entity " #type "(%02x)\n", (val));		\
+		dev_vdbg(ffs->dev, "%s: entity " #type "(%02x)\n", ffs->dev_name, (val));	\
 		if (!__entity_check_ ##type(val)) {			\
-			dev_vdbg(ffs->dev, "invalid entity's value\n");		\
+			dev_vdbg(ffs->dev, "%s: invalid entity's value\n", ffs->dev_name);	\
 			return -EINVAL;					\
 		}							\
 		ret = entity(ffs, FFS_ ##type, &val, _ds, priv);		\
 		if (ret < 0) {						\
-			dev_dbg(ffs->dev, "entity " #type "(%02x); ret = %d\n",	\
-				 (val), ret);				\
+			dev_dbg(ffs->dev, "%s: entity " #type "(%02x); ret = %d\n",	\
+				 ffs->dev_name, (val), ret);		\
 			return ret;					\
 		}							\
 	} while (0)
@@ -2041,13 +2041,13 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 	case USB_DT_STRING:
 	case USB_DT_DEVICE_QUALIFIER:
 		/* function can't have any of those */
-		dev_vdbg(ffs->dev, "descriptor reserved for gadget: %d\n",
-		      _ds->bDescriptorType);
+		dev_vdbg(ffs->dev, "%s: descriptor reserved for gadget: %d\n",
+		      ffs->dev_name, _ds->bDescriptorType);
 		return -EINVAL;
 
 	case USB_DT_INTERFACE: {
 		struct usb_interface_descriptor *ds = (void *)_ds;
-		dev_vdbg(ffs->dev, "interface descriptor\n");
+		dev_vdbg(ffs->dev, "%s: interface descriptor\n", ffs->dev_name);
 		if (length != sizeof *ds)
 			goto inv_length;
 
@@ -2060,7 +2060,7 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 
 	case USB_DT_ENDPOINT: {
 		struct usb_endpoint_descriptor *ds = (void *)_ds;
-		dev_vdbg(ffs->dev, "endpoint descriptor\n");
+		dev_vdbg(ffs->dev, "%s: endpoint descriptor\n", ffs->dev_name);
 		if (length != USB_DT_ENDPOINT_SIZE &&
 		    length != USB_DT_ENDPOINT_AUDIO_SIZE)
 			goto inv_length;
@@ -2070,18 +2070,18 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 
 	case USB_TYPE_CLASS | 0x01:
 		if (*current_class == USB_INTERFACE_CLASS_HID) {
-			dev_vdbg(ffs->dev, "hid descriptor\n");
+			dev_vdbg(ffs->dev, "%s: hid descriptor\n", ffs->dev_name);
 			if (length != sizeof(struct hid_descriptor))
 				goto inv_length;
 			break;
 		} else if (*current_class == USB_INTERFACE_CLASS_CCID) {
-			dev_vdbg(ffs->dev, "ccid descriptor\n");
+			dev_vdbg(ffs->dev, "%s: ccid descriptor\n", ffs->dev_name);
 			if (length != sizeof(struct ccid_descriptor))
 				goto inv_length;
 			break;
 		} else {
-			dev_vdbg(ffs->dev, "unknown descriptor: %d for class %d\n",
-			      _ds->bDescriptorType, *current_class);
+			dev_vdbg(ffs->dev, "%s: unknown descriptor: %d for class %d\n",
+			      ffs->dev_name, _ds->bDescriptorType, *current_class);
 			return -EINVAL;
 		}
 
@@ -2092,7 +2092,7 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 
 	case USB_DT_INTERFACE_ASSOCIATION: {
 		struct usb_interface_assoc_descriptor *ds = (void *)_ds;
-		dev_vdbg(ffs->dev, "interface association descriptor\n");
+		dev_vdbg(ffs->dev, "%s: interface association descriptor\n", ffs->dev_name);
 		if (length != sizeof *ds)
 			goto inv_length;
 		if (ds->iFunction)
@@ -2101,7 +2101,7 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 		break;
 
 	case USB_DT_SS_ENDPOINT_COMP:
-		dev_vdbg(ffs->dev, "EP SS companion descriptor\n");
+		dev_vdbg(ffs->dev, "%s: EP SS companion descriptor\n", ffs->dev_name);
 		if (length != sizeof(struct usb_ss_ep_comp_descriptor))
 			goto inv_length;
 		break;
@@ -2112,17 +2112,19 @@ static int __must_check ffs_do_single_desc(struct ffs_data *ffs, char *data, uns
 	case USB_DT_SECURITY:
 	case USB_DT_CS_RADIO_CONTROL:
 		/* TODO */
-		dev_vdbg(ffs->dev, "unimplemented descriptor: %d\n", _ds->bDescriptorType);
+		dev_vdbg(ffs->dev, "%s: unimplemented descriptor: %d\n", ffs->dev_name,
+			_ds->bDescriptorType);
 		return -EINVAL;
 
 	default:
 		/* We should never be here */
-		dev_vdbg(ffs->dev, "unknown descriptor: %d\n", _ds->bDescriptorType);
+		dev_vdbg(ffs->dev, "%s: unknown descriptor: %d\n", ffs->dev_name,
+				_ds->bDescriptorType);
 		return -EINVAL;
 
 inv_length:
-		dev_vdbg(ffs->dev, "invalid length: %d (descriptor %d)\n",
-			  _ds->bLength, _ds->bDescriptorType);
+		dev_vdbg(ffs->dev, "%s: invalid length: %d (descriptor %d)\n",
+			  ffs->dev_name, _ds->bLength, _ds->bDescriptorType);
 		return -EINVAL;
 	}
 
@@ -2151,8 +2153,8 @@ static int __must_check ffs_do_descs(struct ffs_data *ffs, unsigned count, char
 		/* Record "descriptor" entity */
 		ret = entity(ffs, FFS_DESCRIPTOR, (u8 *)num, (void *)data, priv);
 		if (ret < 0) {
-			dev_dbg(ffs->dev, "entity DESCRIPTOR(%02lx); ret = %d\n",
-				 num, ret);
+			dev_dbg(ffs->dev, "%s: entity DESCRIPTOR(%02lx); ret = %d\n",
+				 ffs->dev_name, num, ret);
 			return ret;
 		}
 
@@ -2162,7 +2164,7 @@ static int __must_check ffs_do_descs(struct ffs_data *ffs, unsigned count, char
 		ret = ffs_do_single_desc(ffs, data, len, entity, priv,
 			&current_class);
 		if (ret < 0) {
-			dev_dbg(ffs->dev, "%s returns %d\n", __func__, ret);
+			dev_dbg(ffs->dev, "%s: %s(): returns %d\n", ffs->dev_name, __func__, ret);
 			return ret;
 		}
 
@@ -2227,11 +2229,12 @@ static int __ffs_do_os_desc_header(struct ffs_data *ffs, enum ffs_os_desc_type *
 	u16 w_index = le16_to_cpu(desc->wIndex);
 
 	if (bcd_version == 0x1) {
-		dev_warn(ffs->dev, "bcdVersion must be 0x0100, stored in Little Endian order. "
-			"Userspace driver should be fixed, accepting 0x0001 for compatibility.\n");
+		dev_warn(ffs->dev, "%s: bcdVersion must be 0x0100, stored in Little Endian order. "
+			"Userspace driver should be fixed, accepting 0x0001 for compatibility.\n",
+			ffs->dev_name);
 	} else if (bcd_version != 0x100) {
-		dev_vdbg(ffs->dev, "unsupported os descriptors version: 0x%x\n",
-			  bcd_version);
+		dev_vdbg(ffs->dev, "%s: unsupported os descriptors version: 0x%x\n",
+			  ffs->dev_name, bcd_version);
 		return -EINVAL;
 	}
 	switch (w_index) {
@@ -2242,7 +2245,7 @@ static int __ffs_do_os_desc_header(struct ffs_data *ffs, enum ffs_os_desc_type *
 		*next_type = FFS_OS_DESC_EXT_PROP;
 		break;
 	default:
-		dev_vdbg(ffs->dev, "unsupported os descriptor type: %d", w_index);
+		dev_vdbg(ffs->dev, "%s: unsupported os descriptor type: %d", ffs->dev_name, w_index);
 		return -EINVAL;
 	}
 
@@ -2267,7 +2270,8 @@ static int __must_check ffs_do_single_os_desc(struct ffs_data *ffs, char *data,
 	while (feature_count--) {
 		ret = entity(ffs, type, h, data, len, priv);
 		if (ret < 0) {
-			dev_dbg(ffs->dev, "bad OS descriptor, type: %d\n", type);
+			dev_dbg(ffs->dev, "%s: bad OS descriptor, type: %d\n",
+					ffs->dev_name, type);
 			return ret;
 		}
 		data += ret;
@@ -2305,8 +2309,8 @@ static int __must_check ffs_do_os_descs(struct ffs_data *ffs, unsigned count,
 
 		ret = __ffs_do_os_desc_header(ffs, &type, desc);
 		if (ret < 0) {
-			dev_dbg(ffs->dev, "entity OS_DESCRIPTOR(%02lx); ret = %d\n",
-				 num, ret);
+			dev_dbg(ffs->dev, "%s: entity OS_DESCRIPTOR(%02lx); ret = %d\n",
+				 ffs->dev_name, num, ret);
 			return ret;
 		}
 		/*
@@ -2326,7 +2330,7 @@ static int __must_check ffs_do_os_descs(struct ffs_data *ffs, unsigned count,
 		ret = ffs_do_single_os_desc(ffs, data, len, type,
 					    feature_count, entity, priv, desc);
 		if (ret < 0) {
-			dev_dbg(ffs->dev, "%s returns %d\n", __func__, ret);
+			dev_dbg(ffs->dev, "%s: %s() returns %d\n", ffs->dev_name, __func__, ret);
 			return ret;
 		}
 
@@ -2360,7 +2364,7 @@ static int __ffs_data_do_os_desc(struct ffs_data *ffs, enum ffs_os_desc_type typ
 			 * values.  We fix it here to avoid returning EINVAL
 			 * in response to values we used to accept.
 			 */
-			dev_dbg(ffs->dev, "usb_ext_compat_desc::Reserved1 forced to 1\n");
+			dev_dbg(ffs->dev, "%s: usb_ext_compat_desc::Reserved1 forced to 1\n", ffs->dev_name);
 			d->Reserved1 = 1;
 		}
 		for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
@@ -2383,20 +2387,20 @@ static int __ffs_data_do_os_desc(struct ffs_data *ffs, enum ffs_os_desc_type typ
 		type = le32_to_cpu(d->dwPropertyDataType);
 		if (type < USB_EXT_PROP_UNICODE ||
 		    type > USB_EXT_PROP_UNICODE_MULTI) {
-			dev_vdbg(ffs->dev, "unsupported os descriptor property type: %d",
-				  type);
+			dev_vdbg(ffs->dev, "%s: unsupported os descriptor property type: %d",
+				  ffs->dev_name, type);
 			return -EINVAL;
 		}
 		pnl = le16_to_cpu(d->wPropertyNameLength);
 		if (length < 14 + pnl) {
-			dev_vdbg(ffs->dev, "invalid os descriptor length: %d pnl:%d (descriptor %d)\n",
-				  length, pnl, type);
+			dev_vdbg(ffs->dev, "%s: invalid os descriptor length: %d pnl:%d (%d)\n",
+				  ffs->dev_name, length, pnl, type);
 			return -EINVAL;
 		}
 		pdl = le32_to_cpu(*(__le32 *)((u8 *)data + 10 + pnl));
 		if (length != 14 + pnl + pdl) {
-			dev_vdbg(ffs->dev, "invalid os descriptor length: %d pnl:%d pdl:%d (descriptor %d)\n",
-				  length, pnl, pdl, type);
+			dev_vdbg(ffs->dev, "%s: invalid os descriptor length: %d pnl:%d pdl:%d (%d)\n",
+				ffs->dev_name, length, pnl, pdl, type);
 			return -EINVAL;
 		}
 		++ffs->ms_os_descs_ext_prop_count;
@@ -2406,7 +2410,7 @@ static int __ffs_data_do_os_desc(struct ffs_data *ffs, enum ffs_os_desc_type typ
 	}
 		break;
 	default:
-		dev_vdbg(ffs->dev, "unknown descriptor: %d\n", type);
+		dev_vdbg(ffs->dev, "%s: unknown descriptor: %d\n", ffs->dev_name, type);
 		return -EINVAL;
 	}
 	return length;
@@ -2721,7 +2725,7 @@ static void __ffs_event_add(struct ffs_data *ffs,
 		break;
 
 	default:
-		WARN(1, "%d: unknown event, this should not happen\n", type);
+		WARN(1, "%s: %d: unknown event, this should not happen\n", ffs->dev_name, type);
 		return;
 	}
 
@@ -2732,11 +2736,11 @@ static void __ffs_event_add(struct ffs_data *ffs,
 			if ((*ev == rem_type1 || *ev == rem_type2) == neg)
 				*out++ = *ev;
 			else
-				dev_vdbg(ffs->dev, "purging event %d\n", *ev);
+				dev_vdbg(ffs->dev, "%s: purging event %d\n", ffs->dev_name, *ev);
 		ffs->ev.count = out - ffs->ev.types;
 	}
 
-	dev_vdbg(ffs->dev, "adding event %d\n", type);
+	dev_vdbg(ffs->dev, "%s: adding event %d\n", ffs->dev_name, type);
 	ffs->ev.types[ffs->ev.count++] = type;
 	wake_up_locked(&ffs->ev.waitq);
 	if (ffs->ffs_eventfd)
@@ -2805,8 +2809,8 @@ static int __ffs_func_bind_do_descs(struct ffs_data *ffs, enum ffs_entity_type t
 	ffs_ep = func->eps + idx;
 
 	if (ffs_ep->descs[ep_desc_id]) {
-		dev_err(ffs->dev, "two %sspeed descriptors for EP %d\n",
-			  speed_names[ep_desc_id],
+		dev_err(ffs->dev, "%s: two %sspeed descriptors for EP %d\n",
+			  ffs->dev_name, speed_names[ep_desc_id],
 			  ds->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK);
 		return -EINVAL;
 	}
@@ -2833,7 +2837,7 @@ static int __ffs_func_bind_do_descs(struct ffs_data *ffs, enum ffs_entity_type t
 		 * endpoint descriptors as if they were full speed.
 		 */
 		wMaxPacketSize = ds->wMaxPacketSize;
-		dev_vdbg(ffs->dev, "autoconfig\n");
+		dev_vdbg(ffs->dev, "%s: autoconfig\n", ffs->dev_name);
 		ep = usb_ep_autoconfig(func->gadget, ds);
 		if (!ep)
 			return -ENOTSUPP;
@@ -2914,7 +2918,7 @@ static int __ffs_func_bind_do_nums(struct ffs_data *ffs, enum ffs_entity_type ty
 		break;
 	}
 
-	dev_vdbg(ffs->dev, "%02x -> %02x\n", *valuep, newValue);
+	dev_vdbg(ffs->dev, "%s: %02x -> %02x\n", ffs->dev_name, *valuep, newValue);
 	*valuep = newValue;
 	return 0;
 }
@@ -2992,7 +2996,7 @@ static int __ffs_func_bind_do_os_desc(struct ffs_data *ffs, enum ffs_os_desc_typ
 	}
 		break;
 	default:
-		dev_vdbg(ffs->dev, "unknown descriptor: %d\n", type);
+		dev_vdbg(ffs->dev, "%s: unknown descriptor: %d\n", ffs->dev_name, type);
 	}
 
 	return length;
@@ -3286,11 +3290,16 @@ static int ffs_func_setup(struct usb_function *f,
 	unsigned long flags;
 	int ret;
 
-	dev_vdbg(ffs->dev, "creq->bRequestType = %02x\n", creq->bRequestType);
-	dev_vdbg(ffs->dev, "creq->bRequest     = %02x\n", creq->bRequest);
-	dev_vdbg(ffs->dev, "creq->wValue       = %04x\n", le16_to_cpu(creq->wValue));
-	dev_vdbg(ffs->dev, "creq->wIndex       = %04x\n", le16_to_cpu(creq->wIndex));
-	dev_vdbg(ffs->dev, "creq->wLength      = %04x\n", le16_to_cpu(creq->wLength));
+	dev_vdbg(ffs->dev, "%s: creq->bRequestType = %02x\n",
+			ffs->dev_name, creq->bRequestType);
+	dev_vdbg(ffs->dev, "%s: creq->bRequest     = %02x\n",
+			ffs->dev_name, creq->bRequest);
+	dev_vdbg(ffs->dev, "%s: creq->wValue       = %04x\n",
+			ffs->dev_name, le16_to_cpu(creq->wValue));
+	dev_vdbg(ffs->dev, "%s: creq->wIndex       = %04x\n",
+			ffs->dev_name, le16_to_cpu(creq->wIndex));
+	dev_vdbg(ffs->dev, "%s: creq->wLength      = %04x\n",
+			ffs->dev_name, le16_to_cpu(creq->wLength));
 
 	/*
 	 * Most requests directed to interface go through here
@@ -3793,7 +3802,7 @@ static char *ffs_prepare_buffer(struct ffs_data *ffs, const char __user *buf, si
 	if (IS_ERR(data))
 		return data;
 
-	dev_vdbg(ffs->dev, "Buffer from user space:\n");
+	dev_vdbg(ffs->dev, "%s: Buffer from user space:\n", ffs->dev_name);
 	ffs_dump_mem("", data, len);
 
 	return data;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev
  2023-03-27 10:12 ` [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev Linyu Yuan
@ 2023-03-29  6:53   ` Greg Kroah-Hartman
  2023-03-29  7:00     ` Linyu Yuan
  0 siblings, 1 reply; 21+ messages in thread
From: Greg Kroah-Hartman @ 2023-03-29  6:53 UTC (permalink / raw)
  To: Linyu Yuan; +Cc: linux-usb

On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote:
> It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer.
> 
> Add a struct device *dev member in struct ffs_data, set it to NULL before
> binding or after unbinding to a usb_gadget, set it reference of usb_gadget
> ->dev when bind success.
> 
> Then it can help replace private pr_vdebug() to dev_vdbg() consistently.
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v3: new patch in this version
> 
>  drivers/usb/gadget/function/f_fs.c | 3 +++
>  drivers/usb/gadget/function/u_fs.h | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index a4051c8..25461f1 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
>  		return NULL;
>  	}
>  
> +	ffs->dev = NULL;
>  	refcount_set(&ffs->ref, 1);
>  	atomic_set(&ffs->opened, 0);
>  	ffs->state = FFS_READ_DESCRIPTORS;
> @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
>  	}
>  
>  	ffs->gadget = cdev->gadget;
> +	ffs->dev = &cdev->gadget->dev;
>  	ffs_data_get(ffs);
>  	return 0;
>  }
> @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs)
>  		mutex_lock(&ffs->mutex);
>  		usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req);
>  		ffs->ep0req = NULL;
> +		ffs->dev = NULL;
>  		ffs->gadget = NULL;
>  		clear_bit(FFS_FL_BOUND, &ffs->flags);
>  		mutex_unlock(&ffs->mutex);
> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
> index 4b3365f..c5f6167 100644
> --- a/drivers/usb/gadget/function/u_fs.h
> +++ b/drivers/usb/gadget/function/u_fs.h
> @@ -146,6 +146,7 @@ enum ffs_setup_state {
>  
>  struct ffs_data {
>  	struct usb_gadget		*gadget;
> +	struct device			*dev;

No, sorry, this is not correct.

You already have a struct device right there in the struct usb_gadget.
Use that one instead, as you are just setting this pointer to the same
value (see above where you set it.)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 6/6] usb: gadget: f_fs: show instance name in debug message
  2023-03-27 10:12 ` [PATCH v3 6/6] usb: gadget: f_fs: show instance name in debug message Linyu Yuan
@ 2023-03-29  6:54   ` Greg Kroah-Hartman
  2023-03-29  7:11     ` Linyu Yuan
  0 siblings, 1 reply; 21+ messages in thread
From: Greg Kroah-Hartman @ 2023-03-29  6:54 UTC (permalink / raw)
  To: Linyu Yuan; +Cc: linux-usb

On Mon, Mar 27, 2023 at 06:12:20PM +0800, Linyu Yuan wrote:
> show ffs->dev_name in all possible debug message.
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v3: change according comments
> v2: split to several changes according to v1 comments
> v1: https://lore.kernel.org/linux-usb/1679481369-30094-1-git-send-email-quic_linyyuan@quicinc.com/
> 
>  drivers/usb/gadget/function/f_fs.c | 141 ++++++++++++++++++++-----------------
>  1 file changed, 75 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 0761eaa..383343d 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -317,12 +317,12 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
>  static int __ffs_ep0_stall(struct ffs_data *ffs)
>  {
>  	if (ffs->ev.can_stall) {
> -		dev_vdbg(ffs->dev, "ep0 stall\n");
> +		dev_vdbg(ffs->dev, "%s: ep0 stall\n", ffs->dev_name);

You already have the name here, it's in the usb-gadget structure, why do
you need to print it out again?

What is the before and after output of this change?  I think it should
have the same information already in it.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 5/6] usb: gadget: f_fs: replace private pr_vdebug() with dev_vdbg
  2023-03-27 10:12 ` [PATCH v3 5/6] usb: gadget: f_fs: replace private pr_vdebug() with dev_vdbg Linyu Yuan
@ 2023-03-29  6:55   ` Greg Kroah-Hartman
  2023-03-29  7:01     ` Linyu Yuan
  0 siblings, 1 reply; 21+ messages in thread
From: Greg Kroah-Hartman @ 2023-03-29  6:55 UTC (permalink / raw)
  To: Linyu Yuan; +Cc: linux-usb

On Mon, Mar 27, 2023 at 06:12:19PM +0800, Linyu Yuan wrote:
> Use command dev_vdbg() macro to show some debug message.
> 
> Also replace some pr_debug/err/warn/info() to dev_dbg/err/warn/info().
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v3: new patch in this version
> 
>  drivers/usb/gadget/function/f_fs.c | 98 +++++++++++++++++++-------------------
>  drivers/usb/gadget/function/u_fs.h |  6 ---
>  2 files changed, 49 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 25461f1..0761eaa 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -317,12 +317,12 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
>  static int __ffs_ep0_stall(struct ffs_data *ffs)
>  {
>  	if (ffs->ev.can_stall) {
> -		pr_vdebug("ep0 stall\n");
> +		dev_vdbg(ffs->dev, "ep0 stall\n");
>  		usb_ep_set_halt(ffs->gadget->ep0);
>  		ffs->setup_state = FFS_NO_SETUP;
>  		return -EL2HLT;
>  	} else {
> -		pr_debug("bogus ep0 stall!\n");
> +		dev_dbg(ffs->dev, "bogus ep0 stall!\n");
>  		return -ESRCH;
>  	}
>  }
> @@ -361,7 +361,7 @@ static ssize_t ffs_ep0_write(struct file *file, const char __user *buf,
>  
>  		/* Handle data */
>  		if (ffs->state == FFS_READ_DESCRIPTORS) {
> -			pr_info("read descriptors\n");
> +			dev_info(ffs->dev, "read descriptors\n");

When a driver works properly, it should be quiet.  Why is this driver
being noisy for normal operations?  Shouldn't these types of messages be
moved to be debugging only?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev
  2023-03-29  6:53   ` Greg Kroah-Hartman
@ 2023-03-29  7:00     ` Linyu Yuan
  2023-03-29  7:31       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 21+ messages in thread
From: Linyu Yuan @ 2023-03-29  7:00 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb


On 3/29/2023 2:53 PM, Greg Kroah-Hartman wrote:
> On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote:
>> It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer.
>>
>> Add a struct device *dev member in struct ffs_data, set it to NULL before
>> binding or after unbinding to a usb_gadget, set it reference of usb_gadget
>> ->dev when bind success.
>>
>> Then it can help replace private pr_vdebug() to dev_vdbg() consistently.
>>
>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>> ---
>> v3: new patch in this version
>>
>>   drivers/usb/gadget/function/f_fs.c | 3 +++
>>   drivers/usb/gadget/function/u_fs.h | 1 +
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index a4051c8..25461f1 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
>>   		return NULL;
>>   	}
>>   
>> +	ffs->dev = NULL;
>>   	refcount_set(&ffs->ref, 1);
>>   	atomic_set(&ffs->opened, 0);
>>   	ffs->state = FFS_READ_DESCRIPTORS;
>> @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
>>   	}
>>   
>>   	ffs->gadget = cdev->gadget;
>> +	ffs->dev = &cdev->gadget->dev;
>>   	ffs_data_get(ffs);
>>   	return 0;
>>   }
>> @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs)
>>   		mutex_lock(&ffs->mutex);
>>   		usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req);
>>   		ffs->ep0req = NULL;
>> +		ffs->dev = NULL;
>>   		ffs->gadget = NULL;
>>   		clear_bit(FFS_FL_BOUND, &ffs->flags);
>>   		mutex_unlock(&ffs->mutex);
>> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
>> index 4b3365f..c5f6167 100644
>> --- a/drivers/usb/gadget/function/u_fs.h
>> +++ b/drivers/usb/gadget/function/u_fs.h
>> @@ -146,6 +146,7 @@ enum ffs_setup_state {
>>   
>>   struct ffs_data {
>>   	struct usb_gadget		*gadget;
>> +	struct device			*dev;
> No, sorry, this is not correct.
>
> You already have a struct device right there in the struct usb_gadget.
> Use that one instead, as you are just setting this pointer to the same
> value (see above where you set it.)


just want to use consistent dev_(v)dbg() related macro, to avoid 
reference  usb_gadget->dev

when usb_gadget is NULL.


>
> thanks,
>
> greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 5/6] usb: gadget: f_fs: replace private pr_vdebug() with dev_vdbg
  2023-03-29  6:55   ` Greg Kroah-Hartman
@ 2023-03-29  7:01     ` Linyu Yuan
  2023-03-29  7:31       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 21+ messages in thread
From: Linyu Yuan @ 2023-03-29  7:01 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb


On 3/29/2023 2:55 PM, Greg Kroah-Hartman wrote:
> On Mon, Mar 27, 2023 at 06:12:19PM +0800, Linyu Yuan wrote:
>> Use command dev_vdbg() macro to show some debug message.
>>
>> Also replace some pr_debug/err/warn/info() to dev_dbg/err/warn/info().
>>
>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>> ---
>> v3: new patch in this version
>>
>>   drivers/usb/gadget/function/f_fs.c | 98 +++++++++++++++++++-------------------
>>   drivers/usb/gadget/function/u_fs.h |  6 ---
>>   2 files changed, 49 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 25461f1..0761eaa 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -317,12 +317,12 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
>>   static int __ffs_ep0_stall(struct ffs_data *ffs)
>>   {
>>   	if (ffs->ev.can_stall) {
>> -		pr_vdebug("ep0 stall\n");
>> +		dev_vdbg(ffs->dev, "ep0 stall\n");
>>   		usb_ep_set_halt(ffs->gadget->ep0);
>>   		ffs->setup_state = FFS_NO_SETUP;
>>   		return -EL2HLT;
>>   	} else {
>> -		pr_debug("bogus ep0 stall!\n");
>> +		dev_dbg(ffs->dev, "bogus ep0 stall!\n");
>>   		return -ESRCH;
>>   	}
>>   }
>> @@ -361,7 +361,7 @@ static ssize_t ffs_ep0_write(struct file *file, const char __user *buf,
>>   
>>   		/* Handle data */
>>   		if (ffs->state == FFS_READ_DESCRIPTORS) {
>> -			pr_info("read descriptors\n");
>> +			dev_info(ffs->dev, "read descriptors\n");
> When a driver works properly, it should be quiet.  Why is this driver
> being noisy for normal operations?  Shouldn't these types of messages be
> moved to be debugging only?


just keep original design, if you accept, will change to dev_dbg().


>
> thanks,
>
> greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 6/6] usb: gadget: f_fs: show instance name in debug message
  2023-03-29  6:54   ` Greg Kroah-Hartman
@ 2023-03-29  7:11     ` Linyu Yuan
  2023-03-29  7:37       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 21+ messages in thread
From: Linyu Yuan @ 2023-03-29  7:11 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb


On 3/29/2023 2:54 PM, Greg Kroah-Hartman wrote:
> On Mon, Mar 27, 2023 at 06:12:20PM +0800, Linyu Yuan wrote:
>> show ffs->dev_name in all possible debug message.
>>
>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>> ---
>> v3: change according comments
>> v2: split to several changes according to v1 comments
>> v1: https://lore.kernel.org/linux-usb/1679481369-30094-1-git-send-email-quic_linyyuan@quicinc.com/
>>
>>   drivers/usb/gadget/function/f_fs.c | 141 ++++++++++++++++++++-----------------
>>   1 file changed, 75 insertions(+), 66 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 0761eaa..383343d 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -317,12 +317,12 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
>>   static int __ffs_ep0_stall(struct ffs_data *ffs)
>>   {
>>   	if (ffs->ev.can_stall) {
>> -		dev_vdbg(ffs->dev, "ep0 stall\n");
>> +		dev_vdbg(ffs->dev, "%s: ep0 stall\n", ffs->dev_name);
> You already have the name here, it's in the usb-gadget structure, why do
> you need to print it out again?
>
> What is the before and after output of this change?  I think it should
> have the same information already in it.


you have wrong understanding of usb_gadget->dev and ffs->dev_name,

this is output example,

[11.046519] configfs-gadget.g1 gadget.0: adb: interface descriptor

usb_gadget->dev is gadget.0,

but ffs->dev_name is adb.


>
> thanks,
>
> greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 5/6] usb: gadget: f_fs: replace private pr_vdebug() with dev_vdbg
  2023-03-29  7:01     ` Linyu Yuan
@ 2023-03-29  7:31       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 21+ messages in thread
From: Greg Kroah-Hartman @ 2023-03-29  7:31 UTC (permalink / raw)
  To: Linyu Yuan; +Cc: linux-usb

On Wed, Mar 29, 2023 at 03:01:49PM +0800, Linyu Yuan wrote:
> 
> On 3/29/2023 2:55 PM, Greg Kroah-Hartman wrote:
> > On Mon, Mar 27, 2023 at 06:12:19PM +0800, Linyu Yuan wrote:
> > > Use command dev_vdbg() macro to show some debug message.
> > > 
> > > Also replace some pr_debug/err/warn/info() to dev_dbg/err/warn/info().
> > > 
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > ---
> > > v3: new patch in this version
> > > 
> > >   drivers/usb/gadget/function/f_fs.c | 98 +++++++++++++++++++-------------------
> > >   drivers/usb/gadget/function/u_fs.h |  6 ---
> > >   2 files changed, 49 insertions(+), 55 deletions(-)
> > > 
> > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > > index 25461f1..0761eaa 100644
> > > --- a/drivers/usb/gadget/function/f_fs.c
> > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > @@ -317,12 +317,12 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
> > >   static int __ffs_ep0_stall(struct ffs_data *ffs)
> > >   {
> > >   	if (ffs->ev.can_stall) {
> > > -		pr_vdebug("ep0 stall\n");
> > > +		dev_vdbg(ffs->dev, "ep0 stall\n");
> > >   		usb_ep_set_halt(ffs->gadget->ep0);
> > >   		ffs->setup_state = FFS_NO_SETUP;
> > >   		return -EL2HLT;
> > >   	} else {
> > > -		pr_debug("bogus ep0 stall!\n");
> > > +		dev_dbg(ffs->dev, "bogus ep0 stall!\n");
> > >   		return -ESRCH;
> > >   	}
> > >   }
> > > @@ -361,7 +361,7 @@ static ssize_t ffs_ep0_write(struct file *file, const char __user *buf,
> > >   		/* Handle data */
> > >   		if (ffs->state == FFS_READ_DESCRIPTORS) {
> > > -			pr_info("read descriptors\n");
> > > +			dev_info(ffs->dev, "read descriptors\n");
> > When a driver works properly, it should be quiet.  Why is this driver
> > being noisy for normal operations?  Shouldn't these types of messages be
> > moved to be debugging only?
> 
> 
> just keep original design, if you accept, will change to dev_dbg().

Please do.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev
  2023-03-29  7:00     ` Linyu Yuan
@ 2023-03-29  7:31       ` Greg Kroah-Hartman
  2023-03-29  7:46         ` Linyu Yuan
  0 siblings, 1 reply; 21+ messages in thread
From: Greg Kroah-Hartman @ 2023-03-29  7:31 UTC (permalink / raw)
  To: Linyu Yuan; +Cc: linux-usb

On Wed, Mar 29, 2023 at 03:00:54PM +0800, Linyu Yuan wrote:
> 
> On 3/29/2023 2:53 PM, Greg Kroah-Hartman wrote:
> > On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote:
> > > It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer.
> > > 
> > > Add a struct device *dev member in struct ffs_data, set it to NULL before
> > > binding or after unbinding to a usb_gadget, set it reference of usb_gadget
> > > ->dev when bind success.
> > > 
> > > Then it can help replace private pr_vdebug() to dev_vdbg() consistently.
> > > 
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > ---
> > > v3: new patch in this version
> > > 
> > >   drivers/usb/gadget/function/f_fs.c | 3 +++
> > >   drivers/usb/gadget/function/u_fs.h | 1 +
> > >   2 files changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > > index a4051c8..25461f1 100644
> > > --- a/drivers/usb/gadget/function/f_fs.c
> > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
> > >   		return NULL;
> > >   	}
> > > +	ffs->dev = NULL;
> > >   	refcount_set(&ffs->ref, 1);
> > >   	atomic_set(&ffs->opened, 0);
> > >   	ffs->state = FFS_READ_DESCRIPTORS;
> > > @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
> > >   	}
> > >   	ffs->gadget = cdev->gadget;
> > > +	ffs->dev = &cdev->gadget->dev;
> > >   	ffs_data_get(ffs);
> > >   	return 0;
> > >   }
> > > @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs)
> > >   		mutex_lock(&ffs->mutex);
> > >   		usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req);
> > >   		ffs->ep0req = NULL;
> > > +		ffs->dev = NULL;
> > >   		ffs->gadget = NULL;
> > >   		clear_bit(FFS_FL_BOUND, &ffs->flags);
> > >   		mutex_unlock(&ffs->mutex);
> > > diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
> > > index 4b3365f..c5f6167 100644
> > > --- a/drivers/usb/gadget/function/u_fs.h
> > > +++ b/drivers/usb/gadget/function/u_fs.h
> > > @@ -146,6 +146,7 @@ enum ffs_setup_state {
> > >   struct ffs_data {
> > >   	struct usb_gadget		*gadget;
> > > +	struct device			*dev;
> > No, sorry, this is not correct.
> > 
> > You already have a struct device right there in the struct usb_gadget.
> > Use that one instead, as you are just setting this pointer to the same
> > value (see above where you set it.)
> 
> 
> just want to use consistent dev_(v)dbg() related macro, to avoid reference 
> usb_gadget->dev
> 
> when usb_gadget is NULL.

When will usb_gadget be NULL when you want to print out logging
messages?  You shouldn't be printing out anything during that time
anyway, right?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 6/6] usb: gadget: f_fs: show instance name in debug message
  2023-03-29  7:11     ` Linyu Yuan
@ 2023-03-29  7:37       ` Greg Kroah-Hartman
  2023-03-29  7:42         ` Linyu Yuan
  0 siblings, 1 reply; 21+ messages in thread
From: Greg Kroah-Hartman @ 2023-03-29  7:37 UTC (permalink / raw)
  To: Linyu Yuan; +Cc: linux-usb

On Wed, Mar 29, 2023 at 03:11:14PM +0800, Linyu Yuan wrote:
> 
> On 3/29/2023 2:54 PM, Greg Kroah-Hartman wrote:
> > On Mon, Mar 27, 2023 at 06:12:20PM +0800, Linyu Yuan wrote:
> > > show ffs->dev_name in all possible debug message.
> > > 
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > ---
> > > v3: change according comments
> > > v2: split to several changes according to v1 comments
> > > v1: https://lore.kernel.org/linux-usb/1679481369-30094-1-git-send-email-quic_linyyuan@quicinc.com/
> > > 
> > >   drivers/usb/gadget/function/f_fs.c | 141 ++++++++++++++++++++-----------------
> > >   1 file changed, 75 insertions(+), 66 deletions(-)
> > > 
> > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > > index 0761eaa..383343d 100644
> > > --- a/drivers/usb/gadget/function/f_fs.c
> > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > @@ -317,12 +317,12 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
> > >   static int __ffs_ep0_stall(struct ffs_data *ffs)
> > >   {
> > >   	if (ffs->ev.can_stall) {
> > > -		dev_vdbg(ffs->dev, "ep0 stall\n");
> > > +		dev_vdbg(ffs->dev, "%s: ep0 stall\n", ffs->dev_name);
> > You already have the name here, it's in the usb-gadget structure, why do
> > you need to print it out again?
> > 
> > What is the before and after output of this change?  I think it should
> > have the same information already in it.
> 
> 
> you have wrong understanding of usb_gadget->dev and ffs->dev_name,
> 
> this is output example,
> 
> [11.046519] configfs-gadget.g1 gadget.0: adb: interface descriptor
> 
> usb_gadget->dev is gadget.0,
> 
> but ffs->dev_name is adb.

Isn't there some mapping of gadget name to "dev_name" somewhere else in
the logs?  And what sets dev_name, why isn't that part of the gadget
name already?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 6/6] usb: gadget: f_fs: show instance name in debug message
  2023-03-29  7:37       ` Greg Kroah-Hartman
@ 2023-03-29  7:42         ` Linyu Yuan
  2023-03-29  8:21           ` Greg Kroah-Hartman
  0 siblings, 1 reply; 21+ messages in thread
From: Linyu Yuan @ 2023-03-29  7:42 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb


On 3/29/2023 3:37 PM, Greg Kroah-Hartman wrote:
> On Wed, Mar 29, 2023 at 03:11:14PM +0800, Linyu Yuan wrote:
>> On 3/29/2023 2:54 PM, Greg Kroah-Hartman wrote:
>>> On Mon, Mar 27, 2023 at 06:12:20PM +0800, Linyu Yuan wrote:
>>>> show ffs->dev_name in all possible debug message.
>>>>
>>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>>>> ---
>>>> v3: change according comments
>>>> v2: split to several changes according to v1 comments
>>>> v1: https://lore.kernel.org/linux-usb/1679481369-30094-1-git-send-email-quic_linyyuan@quicinc.com/
>>>>
>>>>    drivers/usb/gadget/function/f_fs.c | 141 ++++++++++++++++++++-----------------
>>>>    1 file changed, 75 insertions(+), 66 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>>>> index 0761eaa..383343d 100644
>>>> --- a/drivers/usb/gadget/function/f_fs.c
>>>> +++ b/drivers/usb/gadget/function/f_fs.c
>>>> @@ -317,12 +317,12 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
>>>>    static int __ffs_ep0_stall(struct ffs_data *ffs)
>>>>    {
>>>>    	if (ffs->ev.can_stall) {
>>>> -		dev_vdbg(ffs->dev, "ep0 stall\n");
>>>> +		dev_vdbg(ffs->dev, "%s: ep0 stall\n", ffs->dev_name);
>>> You already have the name here, it's in the usb-gadget structure, why do
>>> you need to print it out again?
>>>
>>> What is the before and after output of this change?  I think it should
>>> have the same information already in it.
>>
>> you have wrong understanding of usb_gadget->dev and ffs->dev_name,
>>
>> this is output example,
>>
>> [11.046519] configfs-gadget.g1 gadget.0: adb: interface descriptor
>>
>> usb_gadget->dev is gadget.0,
>>
>> but ffs->dev_name is adb.
> Isn't there some mapping of gadget name to "dev_name" somewhere else in
> the logs?  And what sets dev_name, why isn't that part of the gadget
> name already?


ffs->dev_name should be different from gadget name,

as we can create multiple ffs instances (adb, MTP, PTP ...) which work 
on one gadget device (gadget.0).


>
> thanks,
>
> greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev
  2023-03-29  7:31       ` Greg Kroah-Hartman
@ 2023-03-29  7:46         ` Linyu Yuan
  2023-03-29  8:21           ` Greg Kroah-Hartman
  0 siblings, 1 reply; 21+ messages in thread
From: Linyu Yuan @ 2023-03-29  7:46 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb


On 3/29/2023 3:31 PM, Greg Kroah-Hartman wrote:
> On Wed, Mar 29, 2023 at 03:00:54PM +0800, Linyu Yuan wrote:
>> On 3/29/2023 2:53 PM, Greg Kroah-Hartman wrote:
>>> On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote:
>>>> It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer.
>>>>
>>>> Add a struct device *dev member in struct ffs_data, set it to NULL before
>>>> binding or after unbinding to a usb_gadget, set it reference of usb_gadget
>>>> ->dev when bind success.
>>>>
>>>> Then it can help replace private pr_vdebug() to dev_vdbg() consistently.
>>>>
>>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>>>> ---
>>>> v3: new patch in this version
>>>>
>>>>    drivers/usb/gadget/function/f_fs.c | 3 +++
>>>>    drivers/usb/gadget/function/u_fs.h | 1 +
>>>>    2 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>>>> index a4051c8..25461f1 100644
>>>> --- a/drivers/usb/gadget/function/f_fs.c
>>>> +++ b/drivers/usb/gadget/function/f_fs.c
>>>> @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
>>>>    		return NULL;
>>>>    	}
>>>> +	ffs->dev = NULL;
>>>>    	refcount_set(&ffs->ref, 1);
>>>>    	atomic_set(&ffs->opened, 0);
>>>>    	ffs->state = FFS_READ_DESCRIPTORS;
>>>> @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
>>>>    	}
>>>>    	ffs->gadget = cdev->gadget;
>>>> +	ffs->dev = &cdev->gadget->dev;
>>>>    	ffs_data_get(ffs);
>>>>    	return 0;
>>>>    }
>>>> @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs)
>>>>    		mutex_lock(&ffs->mutex);
>>>>    		usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req);
>>>>    		ffs->ep0req = NULL;
>>>> +		ffs->dev = NULL;
>>>>    		ffs->gadget = NULL;
>>>>    		clear_bit(FFS_FL_BOUND, &ffs->flags);
>>>>    		mutex_unlock(&ffs->mutex);
>>>> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
>>>> index 4b3365f..c5f6167 100644
>>>> --- a/drivers/usb/gadget/function/u_fs.h
>>>> +++ b/drivers/usb/gadget/function/u_fs.h
>>>> @@ -146,6 +146,7 @@ enum ffs_setup_state {
>>>>    struct ffs_data {
>>>>    	struct usb_gadget		*gadget;
>>>> +	struct device			*dev;
>>> No, sorry, this is not correct.
>>>
>>> You already have a struct device right there in the struct usb_gadget.
>>> Use that one instead, as you are just setting this pointer to the same
>>> value (see above where you set it.)
>>
>> just want to use consistent dev_(v)dbg() related macro, to avoid reference
>> usb_gadget->dev
>>
>> when usb_gadget is NULL.
> When will usb_gadget be NULL when you want to print out logging
> messages?  You shouldn't be printing out anything during that time
> anyway, right?


when usb_gadget is NULL, there could be debug message because user space 
application

can start configure the ffs instance (like adb ...) for USB 
interface/endpoint/string descriptor.


as dev_dbg related macro is safe to accept NULL, there is no need find 
out when will

usb_gadget is NULL and when will it a valid pointer.


>
> thanks,
>
> greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev
  2023-03-29  7:46         ` Linyu Yuan
@ 2023-03-29  8:21           ` Greg Kroah-Hartman
  2023-03-29  9:20             ` Linyu Yuan
  0 siblings, 1 reply; 21+ messages in thread
From: Greg Kroah-Hartman @ 2023-03-29  8:21 UTC (permalink / raw)
  To: Linyu Yuan; +Cc: linux-usb

On Wed, Mar 29, 2023 at 03:46:45PM +0800, Linyu Yuan wrote:
> 
> On 3/29/2023 3:31 PM, Greg Kroah-Hartman wrote:
> > On Wed, Mar 29, 2023 at 03:00:54PM +0800, Linyu Yuan wrote:
> > > On 3/29/2023 2:53 PM, Greg Kroah-Hartman wrote:
> > > > On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote:
> > > > > It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer.
> > > > > 
> > > > > Add a struct device *dev member in struct ffs_data, set it to NULL before
> > > > > binding or after unbinding to a usb_gadget, set it reference of usb_gadget
> > > > > ->dev when bind success.
> > > > > 
> > > > > Then it can help replace private pr_vdebug() to dev_vdbg() consistently.
> > > > > 
> > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > > > ---
> > > > > v3: new patch in this version
> > > > > 
> > > > >    drivers/usb/gadget/function/f_fs.c | 3 +++
> > > > >    drivers/usb/gadget/function/u_fs.h | 1 +
> > > > >    2 files changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > > > > index a4051c8..25461f1 100644
> > > > > --- a/drivers/usb/gadget/function/f_fs.c
> > > > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > > > @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
> > > > >    		return NULL;
> > > > >    	}
> > > > > +	ffs->dev = NULL;
> > > > >    	refcount_set(&ffs->ref, 1);
> > > > >    	atomic_set(&ffs->opened, 0);
> > > > >    	ffs->state = FFS_READ_DESCRIPTORS;
> > > > > @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
> > > > >    	}
> > > > >    	ffs->gadget = cdev->gadget;
> > > > > +	ffs->dev = &cdev->gadget->dev;
> > > > >    	ffs_data_get(ffs);
> > > > >    	return 0;
> > > > >    }
> > > > > @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs)
> > > > >    		mutex_lock(&ffs->mutex);
> > > > >    		usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req);
> > > > >    		ffs->ep0req = NULL;
> > > > > +		ffs->dev = NULL;
> > > > >    		ffs->gadget = NULL;
> > > > >    		clear_bit(FFS_FL_BOUND, &ffs->flags);
> > > > >    		mutex_unlock(&ffs->mutex);
> > > > > diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
> > > > > index 4b3365f..c5f6167 100644
> > > > > --- a/drivers/usb/gadget/function/u_fs.h
> > > > > +++ b/drivers/usb/gadget/function/u_fs.h
> > > > > @@ -146,6 +146,7 @@ enum ffs_setup_state {
> > > > >    struct ffs_data {
> > > > >    	struct usb_gadget		*gadget;
> > > > > +	struct device			*dev;
> > > > No, sorry, this is not correct.
> > > > 
> > > > You already have a struct device right there in the struct usb_gadget.
> > > > Use that one instead, as you are just setting this pointer to the same
> > > > value (see above where you set it.)
> > > 
> > > just want to use consistent dev_(v)dbg() related macro, to avoid reference
> > > usb_gadget->dev
> > > 
> > > when usb_gadget is NULL.
> > When will usb_gadget be NULL when you want to print out logging
> > messages?  You shouldn't be printing out anything during that time
> > anyway, right?
> 
> 
> when usb_gadget is NULL, there could be debug message because user space
> application
> 
> can start configure the ffs instance (like adb ...) for USB
> interface/endpoint/string descriptor.

But there is a "real" device down there somewhere as there would not be
any way for the driver to be able to talk to the hardware.  So please
use that.

> as dev_dbg related macro is safe to accept NULL, there is no need find out
> when will
> 
> usb_gadget is NULL and when will it a valid pointer.

Don't abuse dev_dbg() like this, that's not going to help out much
overall, and makes no sense to convert to something that is going to
print out incorrect messages (again, there is a device there.)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 6/6] usb: gadget: f_fs: show instance name in debug message
  2023-03-29  7:42         ` Linyu Yuan
@ 2023-03-29  8:21           ` Greg Kroah-Hartman
  2023-03-29  8:46             ` Linyu Yuan
  0 siblings, 1 reply; 21+ messages in thread
From: Greg Kroah-Hartman @ 2023-03-29  8:21 UTC (permalink / raw)
  To: Linyu Yuan; +Cc: linux-usb

On Wed, Mar 29, 2023 at 03:42:08PM +0800, Linyu Yuan wrote:
> 
> On 3/29/2023 3:37 PM, Greg Kroah-Hartman wrote:
> > On Wed, Mar 29, 2023 at 03:11:14PM +0800, Linyu Yuan wrote:
> > > On 3/29/2023 2:54 PM, Greg Kroah-Hartman wrote:
> > > > On Mon, Mar 27, 2023 at 06:12:20PM +0800, Linyu Yuan wrote:
> > > > > show ffs->dev_name in all possible debug message.
> > > > > 
> > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > > > ---
> > > > > v3: change according comments
> > > > > v2: split to several changes according to v1 comments
> > > > > v1: https://lore.kernel.org/linux-usb/1679481369-30094-1-git-send-email-quic_linyyuan@quicinc.com/
> > > > > 
> > > > >    drivers/usb/gadget/function/f_fs.c | 141 ++++++++++++++++++++-----------------
> > > > >    1 file changed, 75 insertions(+), 66 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > > > > index 0761eaa..383343d 100644
> > > > > --- a/drivers/usb/gadget/function/f_fs.c
> > > > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > > > @@ -317,12 +317,12 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
> > > > >    static int __ffs_ep0_stall(struct ffs_data *ffs)
> > > > >    {
> > > > >    	if (ffs->ev.can_stall) {
> > > > > -		dev_vdbg(ffs->dev, "ep0 stall\n");
> > > > > +		dev_vdbg(ffs->dev, "%s: ep0 stall\n", ffs->dev_name);
> > > > You already have the name here, it's in the usb-gadget structure, why do
> > > > you need to print it out again?
> > > > 
> > > > What is the before and after output of this change?  I think it should
> > > > have the same information already in it.
> > > 
> > > you have wrong understanding of usb_gadget->dev and ffs->dev_name,
> > > 
> > > this is output example,
> > > 
> > > [11.046519] configfs-gadget.g1 gadget.0: adb: interface descriptor
> > > 
> > > usb_gadget->dev is gadget.0,
> > > 
> > > but ffs->dev_name is adb.
> > Isn't there some mapping of gadget name to "dev_name" somewhere else in
> > the logs?  And what sets dev_name, why isn't that part of the gadget
> > name already?
> 
> 
> ffs->dev_name should be different from gadget name,
> 
> as we can create multiple ffs instances (adb, MTP, PTP ...) which work on
> one gadget device (gadget.0).

Shouldn't all of those instances have their own struct device pointer?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 6/6] usb: gadget: f_fs: show instance name in debug message
  2023-03-29  8:21           ` Greg Kroah-Hartman
@ 2023-03-29  8:46             ` Linyu Yuan
  0 siblings, 0 replies; 21+ messages in thread
From: Linyu Yuan @ 2023-03-29  8:46 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb


On 3/29/2023 4:21 PM, Greg Kroah-Hartman wrote:
> On Wed, Mar 29, 2023 at 03:42:08PM +0800, Linyu Yuan wrote:
>> On 3/29/2023 3:37 PM, Greg Kroah-Hartman wrote:
>>> On Wed, Mar 29, 2023 at 03:11:14PM +0800, Linyu Yuan wrote:
>>>> On 3/29/2023 2:54 PM, Greg Kroah-Hartman wrote:
>>>>> On Mon, Mar 27, 2023 at 06:12:20PM +0800, Linyu Yuan wrote:
>>>>>> show ffs->dev_name in all possible debug message.
>>>>>>
>>>>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>>>>>> ---
>>>>>> v3: change according comments
>>>>>> v2: split to several changes according to v1 comments
>>>>>> v1: https://lore.kernel.org/linux-usb/1679481369-30094-1-git-send-email-quic_linyyuan@quicinc.com/
>>>>>>
>>>>>>     drivers/usb/gadget/function/f_fs.c | 141 ++++++++++++++++++++-----------------
>>>>>>     1 file changed, 75 insertions(+), 66 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>>>>>> index 0761eaa..383343d 100644
>>>>>> --- a/drivers/usb/gadget/function/f_fs.c
>>>>>> +++ b/drivers/usb/gadget/function/f_fs.c
>>>>>> @@ -317,12 +317,12 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
>>>>>>     static int __ffs_ep0_stall(struct ffs_data *ffs)
>>>>>>     {
>>>>>>     	if (ffs->ev.can_stall) {
>>>>>> -		dev_vdbg(ffs->dev, "ep0 stall\n");
>>>>>> +		dev_vdbg(ffs->dev, "%s: ep0 stall\n", ffs->dev_name);
>>>>> You already have the name here, it's in the usb-gadget structure, why do
>>>>> you need to print it out again?
>>>>>
>>>>> What is the before and after output of this change?  I think it should
>>>>> have the same information already in it.
>>>> you have wrong understanding of usb_gadget->dev and ffs->dev_name,
>>>>
>>>> this is output example,
>>>>
>>>> [11.046519] configfs-gadget.g1 gadget.0: adb: interface descriptor
>>>>
>>>> usb_gadget->dev is gadget.0,
>>>>
>>>> but ffs->dev_name is adb.
>>> Isn't there some mapping of gadget name to "dev_name" somewhere else in
>>> the logs?  And what sets dev_name, why isn't that part of the gadget
>>> name already?
>>
>> ffs->dev_name should be different from gadget name,
>>
>> as we can create multiple ffs instances (adb, MTP, PTP ...) which work on
>> one gadget device (gadget.0).
> Shouldn't all of those instances have their own struct device pointer?


no, there is no need, it is not a real device, just used for mount 
operation and differentiate

each instance.


>
> thanks,
>
> greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev
  2023-03-29  8:21           ` Greg Kroah-Hartman
@ 2023-03-29  9:20             ` Linyu Yuan
  0 siblings, 0 replies; 21+ messages in thread
From: Linyu Yuan @ 2023-03-29  9:20 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb


On 3/29/2023 4:21 PM, Greg Kroah-Hartman wrote:
> On Wed, Mar 29, 2023 at 03:46:45PM +0800, Linyu Yuan wrote:
>> On 3/29/2023 3:31 PM, Greg Kroah-Hartman wrote:
>>> On Wed, Mar 29, 2023 at 03:00:54PM +0800, Linyu Yuan wrote:
>>>> On 3/29/2023 2:53 PM, Greg Kroah-Hartman wrote:
>>>>> On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote:
>>>>>> It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer.
>>>>>>
>>>>>> Add a struct device *dev member in struct ffs_data, set it to NULL before
>>>>>> binding or after unbinding to a usb_gadget, set it reference of usb_gadget
>>>>>> ->dev when bind success.
>>>>>>
>>>>>> Then it can help replace private pr_vdebug() to dev_vdbg() consistently.
>>>>>>
>>>>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>>>>>> ---
>>>>>> v3: new patch in this version
>>>>>>
>>>>>>     drivers/usb/gadget/function/f_fs.c | 3 +++
>>>>>>     drivers/usb/gadget/function/u_fs.h | 1 +
>>>>>>     2 files changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>>>>>> index a4051c8..25461f1 100644
>>>>>> --- a/drivers/usb/gadget/function/f_fs.c
>>>>>> +++ b/drivers/usb/gadget/function/f_fs.c
>>>>>> @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
>>>>>>     		return NULL;
>>>>>>     	}
>>>>>> +	ffs->dev = NULL;
>>>>>>     	refcount_set(&ffs->ref, 1);
>>>>>>     	atomic_set(&ffs->opened, 0);
>>>>>>     	ffs->state = FFS_READ_DESCRIPTORS;
>>>>>> @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
>>>>>>     	}
>>>>>>     	ffs->gadget = cdev->gadget;
>>>>>> +	ffs->dev = &cdev->gadget->dev;
>>>>>>     	ffs_data_get(ffs);
>>>>>>     	return 0;
>>>>>>     }
>>>>>> @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs)
>>>>>>     		mutex_lock(&ffs->mutex);
>>>>>>     		usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req);
>>>>>>     		ffs->ep0req = NULL;
>>>>>> +		ffs->dev = NULL;
>>>>>>     		ffs->gadget = NULL;
>>>>>>     		clear_bit(FFS_FL_BOUND, &ffs->flags);
>>>>>>     		mutex_unlock(&ffs->mutex);
>>>>>> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
>>>>>> index 4b3365f..c5f6167 100644
>>>>>> --- a/drivers/usb/gadget/function/u_fs.h
>>>>>> +++ b/drivers/usb/gadget/function/u_fs.h
>>>>>> @@ -146,6 +146,7 @@ enum ffs_setup_state {
>>>>>>     struct ffs_data {
>>>>>>     	struct usb_gadget		*gadget;
>>>>>> +	struct device			*dev;
>>>>> No, sorry, this is not correct.
>>>>>
>>>>> You already have a struct device right there in the struct usb_gadget.
>>>>> Use that one instead, as you are just setting this pointer to the same
>>>>> value (see above where you set it.)
>>>> just want to use consistent dev_(v)dbg() related macro, to avoid reference
>>>> usb_gadget->dev
>>>>
>>>> when usb_gadget is NULL.
>>> When will usb_gadget be NULL when you want to print out logging
>>> messages?  You shouldn't be printing out anything during that time
>>> anyway, right?
>>
>> when usb_gadget is NULL, there could be debug message because user space
>> application
>>
>> can start configure the ffs instance (like adb ...) for USB
>> interface/endpoint/string descriptor.
> But there is a "real" device down there somewhere as there would not be
> any way for the driver to be able to talk to the hardware.  So please
> use that.


no real device before bind, it only has a file.


>
>> as dev_dbg related macro is safe to accept NULL, there is no need find out
>> when will
>>
>> usb_gadget is NULL and when will it a valid pointer.
> Don't abuse dev_dbg() like this, that's not going to help out much
> overall, and makes no sense to convert to something that is going to
> print out incorrect messages (again, there is a device there.)
>
> thanks,
>
> greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2023-03-29  9:20 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-27 10:12 [PATCH v3 1/6] usb: gadget: ffs: remove ENTER() macro Linyu Yuan
2023-03-27 10:12 ` [PATCH v3 2/6] usb: gadget: f_fs: add more function with struct ffs_data *ffs parameter Linyu Yuan
2023-03-27 10:12 ` [PATCH v3 3/6] usb: gadget: f_fs: remove struct ffs_data *ffs from struct ffs_desc_helper Linyu Yuan
2023-03-27 10:12 ` [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev Linyu Yuan
2023-03-29  6:53   ` Greg Kroah-Hartman
2023-03-29  7:00     ` Linyu Yuan
2023-03-29  7:31       ` Greg Kroah-Hartman
2023-03-29  7:46         ` Linyu Yuan
2023-03-29  8:21           ` Greg Kroah-Hartman
2023-03-29  9:20             ` Linyu Yuan
2023-03-27 10:12 ` [PATCH v3 5/6] usb: gadget: f_fs: replace private pr_vdebug() with dev_vdbg Linyu Yuan
2023-03-29  6:55   ` Greg Kroah-Hartman
2023-03-29  7:01     ` Linyu Yuan
2023-03-29  7:31       ` Greg Kroah-Hartman
2023-03-27 10:12 ` [PATCH v3 6/6] usb: gadget: f_fs: show instance name in debug message Linyu Yuan
2023-03-29  6:54   ` Greg Kroah-Hartman
2023-03-29  7:11     ` Linyu Yuan
2023-03-29  7:37       ` Greg Kroah-Hartman
2023-03-29  7:42         ` Linyu Yuan
2023-03-29  8:21           ` Greg Kroah-Hartman
2023-03-29  8:46             ` Linyu Yuan

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.