linux-kernel - Re: [PATCH v4 07/15] mtd: nand: move Samsung specific init/detection logic in nand

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: <20170111085727.314de956@bbrezillon>
Date:   Wed, 11 Jan 2017 08:57:27 +0100
From:   Boris Brezillon <boris.brezillon@...e-electrons.com>
To:     Marek Vasut <marek.vasut@...il.com>
Cc:     Richard Weinberger <richard@....at>, linux-mtd@...ts.infradead.org,
        David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Cyrille Pitchen <cyrille.pitchen@...el.com>,
        Icenowy Zheng <icenowy@...c.xyz>, Valdis.Kletnieks@...edu,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 07/15] mtd: nand: move Samsung specific
 init/detection logic in nand_samsung.c

On Tue, 10 Jan 2017 20:00:28 +0100
Marek Vasut <marek.vasut@...il.com> wrote:

> On 01/07/2017 08:49 AM, Boris Brezillon wrote:
> > On Sat, 7 Jan 2017 00:53:24 +0100
> > Marek Vasut <marek.vasut@...il.com> wrote:
> >   
> >> On 01/04/2017 06:08 PM, Boris Brezillon wrote:  
> >>> On Wed, 4 Jan 2017 16:14:07 +0100
> >>> Marek Vasut <marek.vasut@...il.com> wrote:
> >>>     
> >>>> On 01/03/2017 02:01 PM, Boris Brezillon wrote:    
> >>>>> Move Samsung specific initialization and detection logic into
> >>>>> nand_samsung.c. This is part of the "separate vendor specific code from
> >>>>> core" cleanup process.
> >>>>>
> >>>>> Signed-off-by: Boris Brezillon <boris.brezillon@...e-electrons.com>      
> >>>>
> >>>> [...]
> >>>>    
> >>>>> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
> >>>>> index b3a332f37e14..05e9366696c9 100644
> >>>>> --- a/drivers/mtd/nand/nand_ids.c
> >>>>> +++ b/drivers/mtd/nand/nand_ids.c
> >>>>> @@ -10,7 +10,7 @@
> >>>>>  #include <linux/mtd/nand.h>
> >>>>>  #include <linux/sizes.h>
> >>>>>  
> >>>>> -#define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS
> >>>>> +#define LP_OPTIONS 0
> >>>>>  #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16)
> >>>>>  
> >>>>>  #define SP_OPTIONS NAND_NEED_READRDY
> >>>>> @@ -169,10 +169,12 @@ struct nand_flash_dev nand_flash_ids[] = {
> >>>>>  };
> >>>>>  
> >>>>>  /* Manufacturer IDs */
> >>>>> +extern const struct nand_manufacturer_ops samsung_nand_manuf_ops;      
> >>>>
> >>>> Is the extern needed ?    
> >>>
> >>> Yes, unless you have another solution. If you remove the extern keyword
> >>> you just redeclare samsung_nand_manuf_ops here, which is not what we
> >>> want.    
> >>
> >> Maybe some accessor function can help ?
> >>  
> > 
> > You mean, in nand_ids.c
> > 
> >     const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops();
> > 
> >     struct nand_manufacturers nand_manuf_ids[] = {
> >     ...
> > 	{NAND_MFR_SAMSUNG, "Samsung", get_samsung_nand_mafuf_ops},
> >     ...
> >     };
> > 
> > and then, in nand_samsung.c
> > 
> >     const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops()
> >     {
> > 	return &samsung_nand_mafuf_ops;
> >     }  
> 
> Yeah, something like that.
> 
> > What's the point of this extra indirection? I mean, in both cases you
> > use a symbol that is not part of the same source file, so you'll have
> > to define this symbol (using a function prototype or an extern object
> > definition).
> > Is this all about fixing checkpatch warnings, or do you have a problem
> > with objects shared between different source files?  
> 
> The later, separating this with an accessor function feels a bit cleaner
> to me than using extern foo.
> 
> > Now, I agree that the current approach is not ideal. A real improvement
> > would be to let the NAND manufacturer drivers (nand_<vendor>.c) register
> > themselves to the core. Something similar to CLK_OF_DECLARE() or
> > IRQCHIP_DECLARE() for example. But that means creating a dedicated
> > section for the nand_manufs_id table, and it's a lot more invasive than
> > the current approach.  
> 
> Well this would be awesome, but this can also be done later. I presume
> you'll get to it eventually anyway, as soon as you'll be annoyed enough
> with the current ugly-ish implementation.
> 

If we plan to rework it this way, I'd like to keep the existing
approach (with the extern) to avoid changing the prototype of
nand_manufacturer once again when we rework the nand_manufacturer
registration logic.

Also note that in v6 I'm keeping a pointer to the nand_manfucturer
object in nand_chip, so that if we ever need to print the manufacturer
name we don't have to search again in the NAND manufacturer table.
After this rework, I no longer store the manufacturer_ops directly in
nand_chip, and have to access them by doing
chip->manufacturer.desc->ops->xxx.

Which means, with your solution, I'll have to do

	ops = nand_get_manufacturer_ops(chip->manufacturer.desc);
	ops->xxx();

instead of

	chip->manufacturer.desc->ops->xxx();