linux-kernel - Re: [PATCH net-next 2/5] net: lan966x: add the basic lan966x 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: <cdb9c0c334823505a2ce499e36be9507112f4298.camel@pengutronix.de>
Date:   Thu, 18 Nov 2021 11:19:22 +0100
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc:     davem@...emloft.net, kuba@...nel.org, robh+dt@...nel.org,
        linux@...linux.org.uk, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 2/5] net: lan966x: add the basic lan966x driver

Hi Horatiu,

On Wed, 2021-11-17 at 22:42 +0100, Horatiu Vultur wrote:
> > On Wed, 2021-11-17 at 10:18 +0100, Horatiu Vultur wrote:
> > > +static int lan966x_reset_switch(struct lan966x *lan966x)
> > > +{
> > > +     struct reset_control *reset;
> > > +     int val = 0;
> > > +     int ret;
> > > +
> > > +     reset = devm_reset_control_get_shared(lan966x->dev, "switch");
> > > +     if (IS_ERR(reset))
> > > +             dev_warn(lan966x->dev, "Could not obtain switch reset: %ld\n",
> > > +                      PTR_ERR(reset));
> > > +     else
> > > +             reset_control_reset(reset);
> > 
> > According to the device tree bindings, both resets are required.
> > I'd expect this to return on error.
> > Is there any chance of the device working with out the switch reset
> > being triggered?
> 
> The only case that I see is if the bootloader triggers this switch
> reset and then when bootloader starts the kernel and doesn't set back
> the switch in reset. Is this a valid scenario or is a bug in the
> bootloader?

I'm not sure. In general, the kernel shouldn't rely on the bootloader to
have put the devices into a certain working state. If the driver will
not work or worse, if register access could hang the system if the
bootloader has passed control to the kernel with the switch held in
reset and no reset control is available to the driver, it should not
continue after failure to get the reset handle.

I'd suggest to just use:

	reset = devm_reset_control_get_shared(lan966x->dev, "switch");
	if (IS_ERR(reset))
		return dev_err_probe(lan966x->dev, PTR_ERR(reset),
				     "Could not obtain switch reset");
	reset_control_reset(reset);

unless you have a good reason to do otherwise.

regards
Philipp