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@xxxxxxxxxx> > > --- > > 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
