Re: [alsa-devel] [PATCH v2 2/5] soundwire: cadence_master: add hw_reset capability in debugfs

[Vinod Koul <vkoul@kernel.org>]

On 21-10-19, 05:20, Pierre-Louis Bossart wrote:
> On 10/20/19 11:04 PM, Vinod Koul wrote:
> > On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
> > > Provide debugfs capability to kick link and devices into hard-reset
> > > (as defined by MIPI). This capability is really useful when some
> > > devices are no longer responsive and/or to check the software handling
> > > of resynchronization.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > ---
> > >   drivers/soundwire/cadence_master.c | 20 ++++++++++++++++++++
> > >   1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> > > index e3d06330d125..5f900cf2acb9 100644
> > > --- a/drivers/soundwire/cadence_master.c
> > > +++ b/drivers/soundwire/cadence_master.c
> > > @@ -340,6 +340,23 @@ static int cdns_reg_show(struct seq_file *s, void *data)
> > >   }
> > >   DEFINE_SHOW_ATTRIBUTE(cdns_reg);
> > > +static int cdns_hw_reset(void *data, u64 value)
> > > +{
> > > +	struct sdw_cdns *cdns = data;
> > > +	int ret;
> > > +
> > > +	if (value != 1)
> > > +		return -EINVAL;
> > > +
> > > +	ret = sdw_cdns_exit_reset(cdns);
> > 
> > So we are performing reset of the device behind the kernel, so I think
> > it makes sense to mark the kernel as tainted.
> 
> This is a bit ironic. This reset is the only way to prove that the
> enumeration is done right and that all the subsystem fully recovers, and yet
> we'd mark the kernel as 'untrustworthy' and all bug reports would be
> ignored.

Nope you dont expect this would be done on a production system and for
you own testing you can choose not to ignore the reports!

> I don't mind doing this but we'd have to agree on a code. The only one I see
> relevant is "taint requested by userspace application", which is not exactly
> super useful.

But it does tell you that userspace did something so watch out!

> > > +	dev_dbg(cdns->dev, "link hw_reset done: %d\n", ret);
> > > +
> > > +	return ret;
> > 
> > We may want to get rid of the debug and do:
> >          return sdw_cdns_exit_reset();
> 
> this debug line is useful, it marks the start of the reset sequence and
> that's valuable information. Remove it and you've lost the ability to
> analyze the dmesg logs. It's even more useful if we mark the kernel as
> tainted as you suggested above.

ok

> 
> > 
> > > +}
> > > +
> > > +DEFINE_DEBUGFS_ATTRIBUTE(cdns_hw_reset_fops, NULL, cdns_hw_reset, "%llu\n");
> > > +
> > >   /**
> > >    * sdw_cdns_debugfs_init() - Cadence debugfs init
> > >    * @cdns: Cadence instance
> > > @@ -348,6 +365,9 @@ DEFINE_SHOW_ATTRIBUTE(cdns_reg);
> > >   void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
> > >   {
> > >   	debugfs_create_file("cdns-registers", 0400, root, cdns, &cdns_reg_fops);
> > > +
> > > +	debugfs_create_file("cdns-hw-reset", 0200, root, cdns,
> > > +			    &cdns_hw_reset_fops);
> > >   }
> > >   EXPORT_SYMBOL_GPL(sdw_cdns_debugfs_init);
> > > -- 
> > > 2.20.1
> > 

-- 
~Vinod