* [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,
¤t_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,
¤t_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,
¤t_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.