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]

Date:   Thu, 18 Nov 2021 13:49:08 +0100
From:   Horatiu Vultur <horatiu.vultur@...rochip.com>
To:     Philipp Zabel <p.zabel@...gutronix.de>
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

The 11/18/2021 11:19, Philipp Zabel wrote:
> 
> 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.

I agree with you. I will do like you suggested in the next version.

> 
> regards
> Philipp

-- 
/Horatiu