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: <20200511135926.3e5c622d@xps13>
Date:   Mon, 11 May 2020 13:59:26 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Christophe Kerello <christophe.kerello@...com>
Cc:     <richard@....at>, <vigneshr@...com>, <robh+dt@...nel.org>,
        <mark.rutland@....com>, <gregkh@...uxfoundation.org>,
        <boris.brezillon@...labora.com>, <linux-mtd@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <devicetree@...r.kernel.org>, <marex@...x.de>
Subject: Re: [PATCH v4 10/10] mtd: rawnand: stm32_fmc2: get resources from
 parent node

Hi Christophe,

Christophe Kerello <christophe.kerello@...com> wrote on Mon, 11 May
2020 12:21:03 +0200:

> Hi Miquel,
> 
> On 5/11/20 11:18 AM, Miquel Raynal wrote:
> > Hi Christophe,
> > 
> > Christophe Kerello <christophe.kerello@...com> wrote on Wed, 6 May 2020
> > 11:11:19 +0200:
> >   
> >> FMC2 EBI support has been added. Common resources (registers base
> >> and clock) are now shared between the 2 drivers. It means that the
> >> common resources should now be found in the parent device when EBI
> >> node is available.
> >>
> >> Signed-off-by: Christophe Kerello <christophe.kerello@...com>
> >> ---  
> > 
> > [...]
> >   
> >> +
> >> +static bool stm32_fmc2_nfc_check_for_parent(struct platform_device *pdev)
> >> +{
> >> +	u32 i;
> >> +	int nb_resources = 0;
> >> +
> >> +	/* Count the number of resources in reg property */
> >> +	for (i = 0; i < pdev->num_resources; i++) {
> >> +		struct resource *res = &pdev->resource[i];
> >> +
> >> +		if (resource_type(res) == IORESOURCE_MEM)
> >> +			nb_resources++;
> >> +	}
> >> +
> >> +	/* Each CS needs 3 resources defined (data, cmd and addr) */
> >> +	if (nb_resources % 3)
> >> +		return false;
> >> +
> >> +	return true;
> >> +}  
> > 
> > This function looks fragile. Why not just checking the compatible
> > string of the parent node?
> >   
> 
> Yes, it is another way to check that we have an EBI parent node.
> 
> In this implementation, I was checking the number of reg tuples.
> In case we have 6, it means that the register base address is defined in the parent node (EBI node).
> In case we have 7, it means that the register base address is defined in the current node (NFC node).

Yes, I understand what you are doing, but I kind of dislike the logic.
Relying on the number of reg tuples is something that can be done (I
used it myself one time), but I think this is more a hack that you do
when you have no other way to differentiate. I guess the proper way
would be to look at the parent's compatible. If it matches what you
expect, then you can store the dev->of_node->parent->dev somewhere in
your controller's structure and then use it to initialize the clock and
regmap. This way you don't have to move anything else in the probe path.


Thanks,
Miquèl

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ