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] [day] [month] [year] [list]
Date:   Sat, 16 Oct 2021 12:18:17 +0200
From:   Hans de Goede <>
To:     Mark Brown <>
Cc:     "Rafael J . Wysocki" <>,
        Mark Gross <>,
        Andy Shevchenko <>,
        Daniel Scally <>,
        Laurent Pinchart <>,
        Mauro Carvalho Chehab <>,
        Liam Girdwood <>,
        Michael Turquette <>,
        Stephen Boyd <>, Len Brown <>,,,,
        Sakari Ailus <>,
        Kate Hsuan <>,,
Subject: Re: [PATCH 05/12] regulator: Introduce tps68470-regulator driver

Hi Mark,

On 10/16/21 12:29 AM, Mark Brown wrote:
> On Fri, Oct 15, 2021 at 10:14:30PM +0200, Hans de Goede wrote:
>> On 10/15/21 9:59 PM, Mark Brown wrote:


>> During that discussion you said that instead we should sinmply
>> directly pass the regulator_init_data, rather then first
>> encoding this in device-properties in a swnode and then
>> decoding those again in the regulator core.
>> And passing the regulator_init_data is exactly what we are doing
>> now; and now again this is not what we should be doing ?
> No, it is not what the driver doing now.  The driver will *optionally*
> check for platform data, but if none is provided or if it doesn't
> configure some of the regulators then the driver will provide some hard
> coded regulator_init_data as a default.  These might be OK on the system
> you're looking at but will also be used on any other system that happens
> to instantiate the driver without platform data where there's no
> guarantee that the information provided will be safe.  These defaults
> that are being provided may use the same structure that gets used for
> platform data but they aren't really platform data.
> Yes, someone could use this on a system that does things in the standard
> fashion where the platform data is getting passed in but if it's ever
> run on any other system then it's going to assume this default platform
> data with these constraints that have been embedded directly into the
> driver without anything to ensure that they make sense on that system.
> Indeed, now I go back and dig out the rest of the series it seems that
> there's some drivers/platforms/x86 code added later which does in fact
> do the right thing for some but not all of the regulators, it supplies
> some platform data which overrides some but not all of this default
> regulator_init_data using platform_data having identified the system
> using DMI information (with configurations quite different to and much
> more restricted than the defaults provided, exactly why defaults are an
> issue).  I'm now even more confused about what the information that's
> there is doing in the driver.  Either it's all unneeded even for your
> system and should just be deleted, or if any of it is needed then it
> should be moved to being initialised in the same place everything else
> is so that it's only used on your system and not on any other system
> that happens to end up running the driver.
> In any case given that your platform does actually have code for
> identifying it and supplying appropriate platform data the driver itself
> can be fixed by just deleting the else case of
> 	if (pdata && pdata->reg_init_data[i])
> 		config.init_data = pdata->reg_init_data[i];
> 	else
> 		config.init_data = &tps68470_init[i];
> and the data structure/macro it uses.  If no configuration is provided
> by the platform then none should be provided to the core, this in turn
> means that the regulator framework won't reconfigure the hardware if it
> doesn't know it's safe to do so.

Ah, ok. The default regulator_init_data in the tps68470_init[]
array was already there in the out of tree version of this driver
Daniel and I started with:

Now that you have pointed this out I would be more then happy to
delete it and I agree that providing this bogus data is not a
good idea.


> The important thing is to get rid of the hard coded defaults for the
> regulator_init_data in the driver itself, if there is regulator_init_data
> in the driver itself then it should be guarded with a DMI or similar
> quirk.  Like I say above I think now I've gone back and dug through the
> rest of the series once the default init_data is gone it's probably OK.

Ok, for the next version of this patch-set I will:

1. Remove the default init_data
2. Change the toplevel comment to be all C++ style matching the SPDX line
3. Add a || COMPILE_TEST to the Kconfig so that this can be compile-tested
   without selecting the INTEL_SKL_INT3472 option

Thank you for taking the time to dive a bit deeper into the patch-set
and make it clear that your objection is the presence of the default
regulator_init_data; and sorry for loosing my cool a bit in my previous



Powered by blists - more mailing lists