linux-kernel - Re: [PATCH v6 12/13] mtd: rawnand: brcmnand: Add support for getting ecc setting from strap

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]

Date: Thu, 29 Feb 2024 11:31:19 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: William Zhang <william.zhang@...adcom.com>
Cc: Linux MTD List <linux-mtd@...ts.infradead.org>, Linux ARM List
 <linux-arm-kernel@...ts.infradead.org>, Broadcom Kernel List
 <bcm-kernel-feedback-list@...adcom.com>, f.fainelli@...il.com,
 kursad.oney@...adcom.com, joel.peshkin@...adcom.com,
 anand.gore@...adcom.com, dregan@...l.com, kamal.dasu@...adcom.com,
 tomer.yacoby@...adcom.com, dan.beygelman@...adcom.com, David Regan
 <dregan@...adcom.com>, linux-kernel@...r.kernel.org, Vignesh Raghavendra
 <vigneshr@...com>, Brian Norris <computersforpeace@...il.com>, Richard
 Weinberger <richard@....at>
Subject: Re: [PATCH v6 12/13] mtd: rawnand: brcmnand: Add support for
 getting ecc setting from strap

Hi William,

william.zhang@...adcom.com wrote on Mon, 26 Feb 2024 12:05:18 -0800:

> On 2/26/24 00:36, Miquel Raynal wrote:
> > Hi William,
> > 
> > william.zhang@...adcom.com wrote on Fri, 23 Feb 2024 09:25:09 -0800:
> >   
> >> Hi Miquel,
> >>
> >> On 2/23/24 01:18, Miquel Raynal wrote:  
> >>> Hi William,
> >>>
> >>> william.zhang@...adcom.com wrote on Thu, 22 Feb 2024 19:47:57 -0800:  
> >>>    >>>> BCMBCA broadband SoC based board design does not specify ecc setting in  
> >>>> dts but rather use the SoC NAND strap info to obtain the ecc strength
> >>>> and spare area size setting. Add brcm,nand-ecc-use-strap dts propety for
> >>>> this purpose and update driver to support this option. However these two
> >>>> options can not be used at the same time.
> >>>>
> >>>> Signed-off-by: William Zhang <william.zhang@...adcom.com>
> >>>> Reviewed-by: David Regan <dregan@...adcom.com>  
> >>>>   >>>  
> >>> FYI I did not receive patches 7, 8, 9, which makes the series numbering
> >>> very odd.  
> >>>    >> I was using the get maintainer script mainly and it sends to the linux MTD list.  I will add your email directly next time.  
> > 
> > Yes, I prefer to be in Cc of the whole series, please.
> >   
> Sure.  And thanks for applying other patches.  Do you want me to just send a new single patch for the update?

Yes just the missing patch.

> >>>>    >> +static int brcmnand_get_sector_size_1k(struct brcmnand_host *host)  
> >>>> +{
> >>>> +	struct brcmnand_controller *ctrl = host->ctrl;
> >>>> +	int sector_size_bit = brcmnand_sector_1k_shift(ctrl);
> >>>> +	u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs,
> >>>> +						  BRCMNAND_CS_ACC_CONTROL);
> >>>> +	u32 acc_control;
> >>>> +
> >>>> +	if (sector_size_bit < 0)
> >>>> +		return 0;
> >>>> +
> >>>> +	acc_control = nand_readreg(ctrl, acc_control_offs);
> >>>> +
> >>>> +	return (acc_control & BIT(sector_size_bit)) >> sector_size_bit;  
> >>>
> >>> FIELD_PREP, FIELD_GET, *please*.  
> >> You probably missed my reply to your comments on the same patch in v5. Here is the link for the post in case it lost in your email:
> >> https://lore.kernel.org/lkml/c145b90c-e9f0-4d82-94cc-baf7bfda5954@gmail.com/T/#m1d911d2f119f3bd345c575a81b60bc2bd8c461eb  
> > 
> > I didn't miss it, but the reason does not sound legitimate to me.
> > Please work on it, it will be so much cleaner.
> >   
> I understand FIELD_PREP/GET is the preferred way of linux accessing the register fields but it requires a constant MASK value and does not apply to our case as we have different versions of the register and have different mask.  There is way to workaround it. i.e defining the multiple constants directly and using these macros with if/else based on reg version. But it is not clean and since we already have helper functions that handle and return different shift/mask value, I see this is a perfect way for our situation and can adapt to future reg version change easily and cleanly.
> 
> >> The mask is not constant here and cause build errors.  

Which errors?

+       acc_control = nand_readreg(ctrl, acc_control_offs);
+       return FIELD_GET(BIT(sector_size_bit), acc_control);

Does not return any error here.

Thanks,
Miquèl