netdev - Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support

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] [day] [month] [year] [list]

Message-ID: <159284015920.1456598.1495380569655598691@kwain>
Date:   Mon, 22 Jun 2020 17:35:59 +0200
From:   Antoine Tenart <antoine.tenart@...tlin.com>
To:     Quentin Schulz <foss@...il.net>
Cc:     davem@...emloft.net, andrew@...n.ch, f.fainelli@...il.com,
        hkallweit1@...il.com, richardcochran@...il.com,
        alexandre.belloni@...tlin.com, UNGLinuxDriver@...rochip.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        thomas.petazzoni@...tlin.com, allan.nielsen@...rochip.com
Subject: Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support

Hi Quentin,

Quoting Quentin Schulz (2020-06-21 19:35:20)
> On 2020-06-19 14:22, Antoine Tenart wrote:
> [...]
> > @@ -999,9 +1553,35 @@ int vsc8584_ptp_probe(struct phy_device *phydev)
> >       if (!vsc8531->ptp)
> >               return -ENOMEM;
> > 
> > +     mutex_init(&vsc8531->phc_lock);
> >       mutex_init(&vsc8531->ts_lock);
> > 
> > +     /* Retrieve the shared load/save GPIO. Request it as non exclusive as
> > +      * the same GPIO can be requested by all the PHYs of the same 
> > package.
> > +      * Ths GPIO must be used with the phc_lock taken (the lock is shared
> 
> Typo + wrong lock named in the comment, instead:
> 
>          * This GPIO must be used with the gpio_lock taken (the lock is shared
> 
> Though technically both are taken when access to the GPIO is requested 
> AFAICT.

That's right, thanks for pointing this out! I'll fix it for v4.

> Also on another note, maybe we could actually make vsc8531->base_addr
> be a part of vsc85xx_shared_private structure.
> 
> We would still need to compute it to pass it to devm_phy_package_join
> but it can easily be returned by vsc8584_get_base_addr instead of the
> current void and it'd put all the things used for all PHYs in the
> package at the same place.

We actually do not use directly the base_addr anymore from within the
driver, thanks to the shared package conversion. We're now using
__phy_package_write/__phy_package_read which are using the base address.

So the move could be to remove it from the vsc8531_private. If we were
to do it, we would need to find a clean solution: it's still part of a
structure as multiple values are computed in vsc8584_get_base_addr, and
it's a lot easier and cleaner to have them all in the same struct. Also,
that have nothing to do with the current series :)

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com