netdev - Re: [PATCH v5 3/6] net: dsa: microchip: ksz8795: move register offsets and shifts to separate struct

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: <20201207214415.GD27410@pengutronix.de>
Date:   Mon, 7 Dec 2020 22:44:15 +0100
From:   Michael Grzeschik <mgr@...gutronix.de>
To:     Tristram.Ha@...rochip.com
Cc:     andrew@...n.ch, f.fainelli@...il.com, davem@...emloft.net,
        kernel@...gutronix.de, netdev@...r.kernel.org,
        matthias.schiffer@...tq-group.com, Woojung.Huh@...rochip.com,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v5 3/6] net: dsa: microchip: ksz8795: move register
 offsets and shifts to separate struct

On Mon, Dec 07, 2020 at 08:02:57PM +0000, Tristram.Ha@...rochip.com wrote:
>> In order to get this driver used with other switches the functions need
>> to use different offsets and register shifts. This patch changes the
>> direct use of the register defines to register description structures,
>> which can be set depending on the chips register layout.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
>>
>> ---
>> v1 -> v4: - extracted this change from bigger previous patch
>> v4 -> v5: - added missing variables in ksz8_r_vlan_entries
>>           - moved shifts, masks and registers to arrays indexed by enums
>>           - using unsigned types where possible
>> ---
>>  drivers/net/dsa/microchip/ksz8.h        |  69 +++++++
>>  drivers/net/dsa/microchip/ksz8795.c     | 261 +++++++++++++++++-------
>>  drivers/net/dsa/microchip/ksz8795_reg.h |  85 --------
>>  3 files changed, 253 insertions(+), 162 deletions(-)
>>  create mode 100644 drivers/net/dsa/microchip/ksz8.h
>
>Sorry for not respond to these patches sooner.
>
>There are 3 older KSZ switch families: KSZ8863/73, KSZ8895/64, and KSZ8795/94.
>The newer KSZ8795 is not considered an upgrade for KSZ8895, so some of
>these switch registers are moved around and some features are dropped.
>
>It is best to have one driver to support all 3 switches, but some operations are
>Incompatible so it may be better to keep the drivers separate for now.
>
>For basic operations those issues may not occur so it seems simple to have
>one driver handling all 3 switches.  I will come up with a list of those
>incompatibilities.

Look into the next patch. I handled many special cases for the ksz8863
in the "net: dsa: microchip: ksz8795: add support for ksz88xx chips".
These cases, including the VLAN, Tagging ... are handled by checking if
the feautre IS_88X3 is set. This can be extended to other types as well.

My first version of the patches was an RFC series that was mentioning
that it is based on your RFC series for the ksz8895.

8863 RFC: https://patchwork.ozlabs.org/project/netdev/cover/20190508211330.19328-1-m.grzeschik@pengutronix.de/

8895 RFC: https://patchwork.ozlabs.org/patch/822712/

I remember, that I was reading the datasheets of all three chips,
8895, 8863 and 8795. After the 8795 series was mainline, the
obvious next step was to get the 8863 into the 8795 code. The result
is this series.

So the obvious question is, how far does your 8895 series differ
from the 8863 switches?

>The tail tag format of KSZ8863 is different from KSZ8895 and KSZ8795, but
>because of the DSA driver implementation that issue never comes up.

Right. In the first four series I kept an extra tail tag patch. But
after cleaning up I figured that the Implementation matched the
one for the KSZ9893. Therefor I reused the tag code.

>> -static void ksz8_from_vlan(u16 vlan, u8 *fid, u8 *member, u8 *valid)
>> +static void ksz8_from_vlan(struct ksz_device *dev, u32 vlan, u8 *fid,
>> +                          u8 *member, u8 *valid)
>>  {
>> -       *fid = vlan & VLAN_TABLE_FID;
>> -       *member = (vlan & VLAN_TABLE_MEMBERSHIP) >>
>> VLAN_TABLE_MEMBERSHIP_S;
>> -       *valid = !!(vlan & VLAN_TABLE_VALID);
>> +       struct ksz8 *ksz8 = dev->priv;
>> +       const u32 *masks = ksz8->masks;
>> +       const u8 *shifts = ksz8->shifts;
>> +
>> +       *fid = vlan & masks[VLAN_TABLE_FID];
>> +       *member = (vlan & masks[VLAN_TABLE_MEMBERSHIP]) >>
>> +                       shifts[VLAN_TABLE_MEMBERSHIP_S];
>> +       *valid = !!(vlan & masks[VLAN_TABLE_VALID]);
>>  }
>>
>> -static void ksz8_to_vlan(u8 fid, u8 member, u8 valid, u16 *vlan)
>> +static void ksz8_to_vlan(struct ksz_device *dev, u8 fid, u8 member, u8
>> valid,
>> +                        u32 *vlan)
>>  {
>> +       struct ksz8 *ksz8 = dev->priv;
>> +       const u32 *masks = ksz8->masks;
>> +       const u8 *shifts = ksz8->shifts;
>> +
>>         *vlan = fid;
>> -       *vlan |= (u16)member << VLAN_TABLE_MEMBERSHIP_S;
>> +       *vlan |= (u16)member << shifts[VLAN_TABLE_MEMBERSHIP_S];
>>         if (valid)
>> -               *vlan |= VLAN_TABLE_VALID;
>> +               *vlan |= masks[VLAN_TABLE_VALID];
>>  }
>>
>>  static void ksz8_r_vlan_entries(struct ksz_device *dev, u16 addr)
>>  {
>> +       struct ksz8 *ksz8 = dev->priv;
>> +       const u8 *shifts = ksz8->shifts;
>>         u64 data;
>>         int i;
>>
>> @@ -418,7 +509,7 @@ static void ksz8_r_vlan_entries(struct ksz_device
>> *dev, u16 addr)
>>         addr *= dev->phy_port_cnt;
>>         for (i = 0; i < dev->phy_port_cnt; i++) {
>>                 dev->vlan_cache[addr + i].table[0] = (u16)data;
>> -               data >>= VLAN_TABLE_S;
>> +               data >>= shifts[VLAN_TABLE];
>>         }
>>  }
>>
>> @@ -454,6 +545,8 @@ static void ksz8_w_vlan_table(struct ksz_device *dev,
>> u16 vid, u32 vlan)
>>
>
>The VLAN table operation in KSZ8863 is completely different from KSZ8795.
>
>> -/**
>> - * VLAN_TABLE_FID                      00-007F007F-007F007F
>> - * VLAN_TABLE_MEMBERSHIP               00-0F800F80-0F800F80
>> - * VLAN_TABLE_VALID                    00-10001000-10001000
>> - */
>> -
>> -#define VLAN_TABLE_FID                 0x007F
>> -#define VLAN_TABLE_MEMBERSHIP          0x0F80
>> -#define VLAN_TABLE_VALID               0x1000
>> -
>> -#define VLAN_TABLE_MEMBERSHIP_S                7
>> -#define VLAN_TABLE_S                   16
>> -
>
>In KSZ8795 you can use all 4096 VLAN id.  Each entry in the table contains
>4 VID.  The FID is actually used for lookup and there is a limit, so you need
>to convert VID to FID when programming the VLAN table.
>
>The only effect of using FID is the same MAC address can be used in
>different VLANs.
>
>In KSZ8863 there are only 16 entries in the VLAN table.  Just like static
>MAC table each entry is programmed using an index.  The entry contains
>the VID, which does not have any relationship with the index unlike in
>KSZ8795.
>
>The number of FID is also limited to 16.  So the maximum VLAN used is 16.
>
>>  /**
>>   * MIB_COUNTER_VALUE                   00-00000000-3FFFFFFF
>>   * MIB_TOTAL_BYTES                     00-0000000F-FFFFFFFF
>> @@ -956,9 +874,6 @@
>>   * MIB_COUNTER_OVERFLOW                        00-00000040-00000000
>>   */
>>
>> -#define MIB_COUNTER_OVERFLOW           BIT(6)
>> -#define MIB_COUNTER_VALID              BIT(5)
>> -
>>  #define MIB_COUNTER_VALUE              0x3FFFFFFF
>
>The MIB counters are also a little different in KSZ8863 and KSZ8795.
>KSZ8863 may have smaller total bytes.  In normal operation this issue may
>not come up.

As mentioned before, these cases are handled differently for both
types of drivers.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)