netdev - Re: [Patch net-next 00/13] net: dsa: microchip: common spi probe for the ksz series switches

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]

Date:   Fri, 24 Jun 2022 14:23:04 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Arun Ramadoss <arun.ramadoss@...rochip.com>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Woojung Huh <woojung.huh@...rochip.com>,
        UNGLinuxDriver@...rochip.com, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>
Subject: Re: [Patch net-next 00/13] net: dsa: microchip: common spi probe for
 the ksz series switches - part 2

On Wed, Jun 22, 2022 at 02:34:12PM +0530, Arun Ramadoss wrote:
> This patch series aims to refactor the ksz_switch_register routine to have the
> common flow for the ksz series switch. And this is the follow up patch series.
> 
> First, it tries moves the common implementation in the setup from individual
> files to ksz_setup. Then implements the common dsa_switch_ops structure instead
> of independent registration. And then moves the ksz_dev_ops to ksz_common.c,
> it allows the dynamic detection of which ksz_dev_ops to be used based on
> the switch detection function.
> 
> Finally, the patch updates the ksz_spi probe function to be same for all the
> ksz_switches.

Sorry for being late to the party again. I've looked over the resulting
code and it appears that there is still some cleanup to do.

We now have a stray struct ksz8 pointer being allocated by the common
ksz_spi_probe(), and passed as dev->priv to ksz_switch_alloc() by this
generic code.

Only ksz8 accesses dev->priv, although it is interesting to note that
ksz9477_i2c_probe() calls ksz_switch_alloc() with a type-incompatible
struct i2c_client *i2c that is unused but bogus.

The concept of struct ksz8 was added by commit 9f73e11250fb ("net: dsa:
microchip: ksz8795: move register offsets and shifts to separate
struct"), and in essence it isn't a bad idea, it's just that I wasn't
aware of it, and only ksz8 makes use of it.

You've added some register offsets yourself to ksz_chip_data
(stp_ctrl_reg, broadcast_ctrl_reg, multicast_ctrl_reg, start_ctrl_reg),
and it looks like struct ksz8 shares more or less the same purpose -
regs, masks, shifts etc. Would you mind doing some more consolidation
work and trying to figure out if we could eliminate a data structure
unique for ksz8 and integrate that information into struct ksz_chip_data
(and perhaps use it in more places)?