linux-kernel - Re: [PATCH 01/13] coresight: cti: Initial CoreSight CTI Driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200318182341.GB3235688@kroah.com>
Date:   Wed, 18 Mar 2020 19:23:41 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:     Mike Leach <mike.leach@...aro.org>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/13] coresight: cti: Initial CoreSight CTI Driver

On Wed, Mar 18, 2020 at 12:12:26PM -0600, Mathieu Poirier wrote:
> > And shouldn't this just be a single value, this looks like it is 2
> > values in one line, that then needs to be parsed, is that to be
> > expected?
> 
> There is no shortage of files under /sys/device/ with output that needs parsing,

Then they need to be fixed.  Seriously, we've been doing this and
fighting this for 15 years, not giving up yet! :)


> but this can be split in two entries.

Please do.

> > Where is the documentation for this new sysfs file?
> 
> All the documentation for sysfs files are lumped together in a single patch [1]
> that is also part of this set.
> 
> [1]. https://lkml.org/lkml/2020/3/9/643

As I reported in the other email, that is not the correct format to use.

> 
> > 
> > > +const struct attribute_group *coresight_cti_groups[] = {
> > > +	&coresight_cti_group,
> > > +	NULL,
> > > +};
> > 
> > ATTRIBUTE_GROUPS()?
> 
> As with all the other coresight devices, groups are communicated to
> coresight_register() and added to the csdev->dev in that function.

Ah, ok, missed that, sorry.

> > > +static struct amba_driver cti_driver = {
> > > +	.drv = {
> > > +		.name	= "coresight-cti",
> > > +		.owner = THIS_MODULE,
> > 
> > Aren't amba drivers smart enough to set this properly on their own?
> > {sigh}
> 
> Would you mind indicating where?  builtin_amba_driver() calls
> amba_driver_register() and  that doesn't set the owner field.

Yes, it doesn't, I'm saying that the amba bus code should be fixed, not
that this is a bug here, just complaining in general :)

thanks,

greg k-h