netdev - Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties

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:   Mon, 13 May 2019 19:58:17 +0000
From:   Trent Piepho <tpiepho@...inj.com>
To:     "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store
 unsigned properties

On Sat, 2019-05-11 at 14:32 +0200, Heiner Kallweit wrote:
> On 11.05.2019 12:41, Heiner Kallweit wrote:
> > On 10.05.2019 23:46, Trent Piepho wrote:
> > > The variables used to store u32 DT properties were signed
> > > ints.  This
> > > doesn't work properly if the value of the property were to
> > > overflow.
> > > Use unsigned variables so this doesn't happen.
> > > 
> > 
> > In patch 3 you added a check for DT properties being out of range.
> > I think this would be good also for the three properties here.
> > The delay values are only 4 bits wide, so you might also consider
> > to switch to u8 or u16.
> > 
> 
> I briefly looked over the rest of the driver. What is plain wrong
> is to allocate memory for the private data structure in the
> config_init callback. This has to be done in the probe callback.
> An example is marvell_probe(). As you seem to work on this driver,
> can you provide a patch for this?

It only allocates the data once, so it is not a memory leak.  But yes,
totally wrong place to do it.  I can fix this.  It also does not set
the signal line impedance from DT value unless unless also configuring
clock skew, even though they are orthogonal concepts.  And fails to
verify anything read from the DT.

Perhaps you could tell me if the approach I've taken in patch 3, 
"Add ability to disable output clock", and patch 4, "Disable tx/rx
delay when not configured", are considered acceptable?  I can conceive
of arguments for alternate approaches.  I would like to add support for
 these into u-boot too, but typically u-boot follows the kernel DT
bindings, so I want to finalize the kernel DT semantics before sending
patches to u-boot.

> > Please note that net-next is closed currently. Please resubmit the
> > patches once it's open again, and please annotate them properly
> > with net-next.

Sorry, didn't know about this policy.  Been years since I've submitted
net patches.  Is there a description somewhere of how this is done? 
Googling net-next wasn't helpful.  I gather new patches are only
allowed when the kernel merge window is open?  And they can't be queued
on patchwork or a topic branch until this happens?