Re: [PATCH v1 1/2] usb: typec: tcpm: Expose tcpm logs through a misc device

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, May 18, 2021 at 04:00:48PM +0300, Heikki Krogerus wrote:
> On Mon, May 17, 2021 at 04:36:08PM -0700, Badhri Jagan Sridharan wrote:
> > Although TCPM logs were primarily focussed to aid developers
> > during bringup, TCPM logs have proved to be critical even
> > post-bringup as it provides a good starting point to triage
> > interoperability issues with accessories. TCPM logs
> > are exposed through debugfs filesystem. For systems that
> > don't mount debugfs by default, this change introduces a
> > module parameter log_misc_dev which when set exports the
> > tcpm logs through a misc device. This change also leaves
> > the option of exporting tcpm logs through debugfs for
> > backwards compatibility.
> 
> This does not look correct to me. At the very least you need to now
> document your misc device interface. Why are you removing the choice
> whether to enable this or not? The module parameter does also not
> sound like a good idea at all.
> 
> I'm really not sure about this. This is just a poor man's debugfs that
> removes any choice of enabling it. Since clearly debugging related
> information is what you are after here, debugfs really should be
> enough for you.

I agree, this is an abuse of a misc device, not ok at all.

Just use debugfs if you really want this type of thing, that is what it
is there for.

> 
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 98 +++++++++++++++++++++++++----------
> >  1 file changed, 72 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index c4fdc00a3bc8..b79194919b27 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/jiffies.h>
> >  #include <linux/kernel.h>
> >  #include <linux/kthread.h>
> > +#include <linux/miscdevice.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/power_supply.h>
> > @@ -33,6 +34,10 @@
> >  
> >  #include <uapi/linux/sched/types.h>
> >  
> > +static bool modparam_log_misc_dev;
> > +module_param_named(log_misc_dev, modparam_log_misc_dev, bool, 0444);
> > +MODULE_PARM_DESC(log_misc_dev, "Expose tcpm logs through misc device");

This is not the 1990's, no new module parameters please.

thanks,

greg k-h