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: <li75hdp527xa3k23za3mfnwgwdcs7j324mlqj3qcxto6t5f6mw@yvhnpxlvlt5c>
Date: Thu, 29 Aug 2024 13:52:40 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Jitendra Vegiraju <jitendra.vegiraju@...adcom.com>
Cc: netdev@...r.kernel.org, alexandre.torgue@...s.st.com, 
	joabreu@...opsys.com, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, mcoquelin.stm32@...il.com, bcm-kernel-feedback-list@...adcom.com, 
	richardcochran@...il.com, ast@...nel.org, daniel@...earbox.net, hawk@...nel.org, 
	john.fastabend@...il.com, rmk+kernel@...linux.org.uk, ahalaney@...hat.com, 
	xiaolei.wang@...driver.com, rohan.g.thomas@...el.com, Jianheng.Zhang@...opsys.com, 
	linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org, bpf@...r.kernel.org, andrew@...n.ch, linux@...linux.org.uk, 
	horms@...nel.org, florian.fainelli@...adcom.com
Subject: Re: [net-next v4 3/5] net: stmmac: Integrate dw25gmac into stmmac
 hwif handling

Hi Jitendra

On Mon, Aug 26, 2024 at 11:53:13AM -0700, Jitendra Vegiraju wrote:
> Hi Serge(y)
> Thank you for reviewing the patch.
> 
> On Fri, Aug 23, 2024 at 6:49 AM Serge Semin <fancer.lancer@...il.com> wrote:
> >
> > Hi Jitendra
> >
> > On Wed, Aug 14, 2024 at 03:18:16PM -0700, jitendra.vegiraju@...adcom.com wrote:
> > > From: Jitendra Vegiraju <jitendra.vegiraju@...adcom.com>
> > >
> > > Integrate dw25gmac support into stmmac hardware interface handling.
> > > Added a new entry to the stmmac_hw table in hwif.c.
> > > Define new macros DW25GMAC_CORE_4_00 and DW25GMAC_ID to identify 25GMAC
> > > device.
> > > Since BCM8958x is an early adaptor device, the synopsis_id reported in HW
> > > is 0x32 and device_id is DWXGMAC_ID. Provide override support by defining
> > > synopsys_dev_id member in struct stmmac_priv so that driver specific setup
> > > functions can override the hardware reported values.
> > >
> > > Signed-off-by: Jitendra Vegiraju <jitendra.vegiraju@...adcom.com>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/common.h |  2 ++
> > >  drivers/net/ethernet/stmicro/stmmac/hwif.c   | 25 ++++++++++++++++++--
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac.h |  1 +
> > >  3 files changed, 26 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> > > index 684489156dce..46edbe73a124 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> > > @@ -38,9 +38,11 @@
> > >  #define DWXGMAC_CORE_2_10    0x21
> > >  #define DWXGMAC_CORE_2_20    0x22
> > >  #define DWXLGMAC_CORE_2_00   0x20
> > > +#define DW25GMAC_CORE_4_00   0x40
> > >
> > >  /* Device ID */
> > >  #define DWXGMAC_ID           0x76
> > > +#define DW25GMAC_ID          0x55
> > >  #define DWXLGMAC_ID          0x27
> > >
> > >  #define STMMAC_CHAN0 0       /* Always supported and default for all chips */
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > index 29367105df54..97e5594ddcda 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > @@ -278,6 +278,27 @@ static const struct stmmac_hwif_entry {
> > >               .est = &dwmac510_est_ops,
> > >               .setup = dwxlgmac2_setup,
> > >               .quirks = stmmac_dwxlgmac_quirks,
> >
> > > +     }, {
> > > +             .gmac = false,
> > > +             .gmac4 = false,
> > > +             .xgmac = true,
> > > +             .min_id = DW25GMAC_CORE_4_00,
> > > +             .dev_id = DW25GMAC_ID,
> > > +             .regs = {
> > > +                     .ptp_off = PTP_XGMAC_OFFSET,
> > > +                     .mmc_off = MMC_XGMAC_OFFSET,
> > > +                     .est_off = EST_XGMAC_OFFSET,
> > > +             },
> > > +             .desc = &dwxgmac210_desc_ops,
> > > +             .dma = &dw25gmac400_dma_ops,
> > > +             .mac = &dwxgmac210_ops,
> > > +             .hwtimestamp = &stmmac_ptp,
> > > +             .mode = NULL,
> > > +             .tc = &dwmac510_tc_ops,
> > > +             .mmc = &dwxgmac_mmc_ops,
> > > +             .est = &dwmac510_est_ops,
> > > +             .setup = dwxgmac2_setup,
> > > +             .quirks = NULL,
> > >       },
> >
> > This can be replaced with just:
> >
> > +       }, {
> > +               .gmac = false,
> > +               .gmac4 = false,
> > +               .xgmac = true,
> > +               .min_id = DW25GMAC_CORE_4_00,
> > +               .dev_id = DWXGMAC_ID, /* Early DW 25GMAC IP-core had XGMAC ID */
> > +               .regs = {
> > +                       .ptp_off = PTP_XGMAC_OFFSET,
> > +                       .mmc_off = MMC_XGMAC_OFFSET,
> > +                       .est_off = EST_XGMAC_OFFSET,
> > +               },
> > +               .desc = &dwxgmac210_desc_ops,
> > +               .dma = &dw25gmac400_dma_ops,
> > +               .mac = &dwxgmac210_ops,
> > +               .hwtimestamp = &stmmac_ptp,
> > +               .mode = NULL,
> > +               .tc = &dwmac510_tc_ops,
> > +               .mmc = &dwxgmac_mmc_ops,
> > +               .est = &dwmac510_est_ops,
> > +               .setup = dw25gmac_setup,
> > +               .quirks = NULL,
> >         }
> >
> > and you won't need to pre-define the setup() method in the
> > glue driver. Instead you can define a new dw25xgmac_setup() method in
> > the dwxgmac2_core.c as it's done for the DW XGMAC/LXGMAC IP-cores.
> >
> > Note if your device is capable to work with up to 10Gbps speed, then
> > just set the plat_stmmacenet_data::max_speed field to SPEED_10000.
> > Alternatively if you really need to specify the exact MAC
> > capabilities, then you can implement what Russell suggested here
> > sometime ago:
> > https://lore.kernel.org/netdev/Zf3ifH%2FCjyHtmXE3@shell.armlinux.org.uk/
> >
> I like your suggestion to add one stmmac_hw[] array entry (entry_a) for this
> "early release" DW25GMAC IP and another entry (entry_b) for final DW25MAC
> IP, in the process eliminate the need for a new member variable in struct
> stmmac_priv.
> 

> However, I would like to bring to your attention that this device requires
> special handling for both synopsys_id and dev_id.
> This device is reporting 0x32 for synopsys_id and 0x76(XGMAC) for dev_id.
> The final 25GMAC spec will have 0x40 for synopsys_id and 0x55(25GMAC) for
> dev_id.

For some reason I was thinking that your device had only the device ID
pre-defined with the XGMAC value meanwhile the Synopsys ID was 0x40.
Indeed you get to override both of these data in the platform-specific
setup() method.

> 
> So, in order to avoid falsely qualifying other XGMAC devices with
> synopsis_id >=0x32 as DW25GMAC, I am thinking we will have to overwrite the
> synopsys_id to 0x40 (DW25GMAC_CORE_4_00) in glue driver using existing
> glue driver override mechanism.
> 
> We can implement dw25gmac_setup() in dwxgmac2_core.c for generic 25GMAC
> case. But, this glue driver will have to rely on its own setup function
> to override the synopsys_id as DW25GMAC_CORE_4_00.
> 
> Do you think it looks reasonable?

What I was trying to avoid was the setup() method re-definition just
for the sake of the IP-core version override. Because if not for that
you could have created and used the generic DW 25GMAC dw25gmac_setup()
function.

One of the possible solutions I was thinking was to introduce the
plat_stmmacenet_data::{snps_id,dev_id} fields and use their values in
the stmmac_hwif_init() procedure instead of the data read from the
MAC.VERSION CSR.

Another solution could be to add the plat_stmmacenet_data::has_25gmac
field and fix the generic driver code to using it where it's relevant.
Then you won't need to think about what actual Synopsys ID/Device ID
since it would mean a whole new IP-core.

-Serge(y)

> If yes, do you want me to add the generic 25GMAC stmmac_hw[] entry
> ("entry_b") now or when
> such a device becomes available for testing?
> 
> > If you also have a DW 25GMAC-based device with 0x55 device ID, then
> > just add another stmmac_hw[] array entry.
> >
> 
> > >  };
> > >
> > >       int hw_cap_support;
> > >       int synopsys_id;
> >
> > > +     int synopsys_dev_id;
> >
> > With the suggestion above implemented you won't need this.
> 
> Got it. Thanks.
> 
> >
> > -Serge(y)
> >
> > >       u32 msg_enable;
> > >       int wolopts;
> > >       int wol_irq;
> > > --
> > > 2.34.1
> > >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ