| 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
| ||
|
Message-ID:
<MN6PR18MB5466C31F8BAB1ADBE1D8BD07D3D8A@MN6PR18MB5466.namprd18.prod.outlook.com>
Date: Tue, 2 Dec 2025 10:45:44 +0000
From: Vimlesh Kumar <vimleshk@...vell.com>
To: Simon Horman <horms@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Sathesh B
Edara <sedara@...vell.com>,
Shinas Rasheed <srasheed@...vell.com>,
Haseeb
Gani <hgani@...vell.com>,
Veerasenareddy Burru <vburru@...vell.com>,
Andrew
Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric
Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni
<pabeni@...hat.com>
Subject: RE: [EXTERNAL] Re: [PATCH net-next v1 1/1] octeon_ep: reset firmware
ready status
> -----Original Message-----
> On Thu, Nov 20, 2025 at 11:23:44AM +0000, Vimlesh Kumar wrote:
> > Add support to reset firmware ready status when the driver is
> > removed(either in unload or unbind)
> >
> > Signed-off-by: Sathesh Edara <sedara@...vell.com>
> > Signed-off-by: Shinas Rasheed <srasheed@...vell.com>
> > Signed-off-by: Vimlesh Kumar <vimleshk@...vell.com>
>
> I'm a little confused about the asymmetry of the cn9k and cnxk code before
> this patch. Maybe it would make sense to split this into two patches, one for
> cn9k and cnxk with more specific commit messages.
>
> And for both cn9k and cnxk, I'm unclear on the what the behaviour was
> before this patch? IOW, is this a bug fix for either of both of xn9k and cnkx?
>
Due to a hardware bug, the firmware-ready status is not cleared as expected. To work around this issue, we introduce the following mechanism to track firmware states:
Initial state: 0
Firmware ready: 1
Driver loaded: 2
Driver unloaded: 0
This patch implements the workaround and ensures proper state tracking.
> > ---
> > .../marvell/octeon_ep/octep_cn9k_pf.c | 22 +++++++++++++++++++
> > .../marvell/octeon_ep/octep_cnxk_pf.c | 2 +-
> > .../marvell/octeon_ep/octep_regs_cn9k_pf.h | 11 ++++++++++
> > .../marvell/octeon_ep/octep_regs_cnxk_pf.h | 1 +
> > 4 files changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c
> > b/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c
> > index b5805969404f..6f926e82c17c 100644
> > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c
> > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c
> > @@ -637,6 +637,17 @@ static int octep_soft_reset_cn93_pf(struct
> > octep_device *oct)
> >
> > octep_write_csr64(oct, CN93_SDP_WIN_WR_MASK_REG, 0xFF);
> >
> > + /* Firmware status CSR is supposed to be cleared by
> > + * core domain reset, but due to a hw bug, it is not.
> > + * Set it to RUNNING right before reset so that it is not
> > + * left in READY (1) state after a reset. This is required
> > + * in addition to the early setting to handle the case where
> > + * the OcteonTX is unexpectedly reset, reboots, and then
> > + * the module is removed.
> > + */
> > + OCTEP_PCI_WIN_WRITE(oct, CN9K_PEMX_PFX_CSX_PFCFGX(0, 0,
> CN9K_PCIEEP_VSECST_CTL),
> > + FW_STATUS_DOWNING);
> > +
>
> There seems to be some inconsistency between the comment, which
> describes setting the status to RUNNING, and the code, which sets the status
> to DOWNING.
>
Commit message describes that we are resetting the firmware ready status as "downing".
> Flagged by Claude Code with
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_masoncl_review-
> 2Dprompts_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=bhf3N5Cr9NFbZwl
> a6EbZhYpRHhQzfrqxMipYIZpCMYA&m=yz6X3FAREG4lEnMm1NZ5tlEbA93LNp
> mAuB-oAD0v5tF4Fpt-
> AJ5SchMh4Has32Uh&s=iwpuufwfypPjLZ6OwImMJdwsVRWs_ylnoi8Mn33WB
> KQ&e=
>
> > /* Set core domain reset bit */
> > OCTEP_PCI_WIN_WRITE(oct, CN93_RST_CORE_DOMAIN_W1S, 1);
> > /* Wait for 100ms as Octeon resets. */ @@ -894,4 +905,15 @@ void
> > octep_device_setup_cn93_pf(struct octep_device *oct)
> >
> > octep_init_config_cn93_pf(oct);
> > octep_configure_ring_mapping_cn93_pf(oct);
> > +
> > + if (oct->chip_id == OCTEP_PCI_DEVICE_ID_CN98_PF)
> > + return;
> > +
> > + /* Firmware status CSR is supposed to be cleared by
> > + * core domain reset, but due to IPBUPEM-38842, it is not.
> > + * Set it to RUNNING early in boot, so that unexpected resets
> > + * leave it in a state that is not READY (1).
> > + */
> > + OCTEP_PCI_WIN_WRITE(oct, CN9K_PEMX_PFX_CSX_PFCFGX(0, 0,
> CN9K_PCIEEP_VSECST_CTL),
> > + FW_STATUS_RUNNING);
> > }
> > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c
> > b/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c
> > index 5de0b5ecbc5f..e07264b3dbf8 100644
> > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c
> > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c
> > @@ -660,7 +660,7 @@ static int octep_soft_reset_cnxk_pf(struct
> octep_device *oct)
> > * the module is removed.
> > */
> > OCTEP_PCI_WIN_WRITE(oct, CNXK_PEMX_PFX_CSX_PFCFGX(0, 0,
> CNXK_PCIEEP_VSECST_CTL),
> > - FW_STATUS_RUNNING);
> > + FW_STATUS_DOWNING);
>
> Likewise here.
>
> >
> > /* Set chip domain reset bit */
> > OCTEP_PCI_WIN_WRITE(oct, CNXK_RST_CHIP_DOMAIN_W1S, 1); diff
> --git
> > a/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cn9k_pf.h
> > b/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cn9k_pf.h
> > index ca473502d7a0..d7fa5adbce98 100644
> > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cn9k_pf.h
> > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cn9k_pf.h
> > @@ -383,6 +383,17 @@
> > /* bit 1 for firmware heartbeat interrupt */
> > #define CN93_SDP_EPF_OEI_RINT_DATA_BIT_HBEAT BIT_ULL(1)
> >
> > +#define FW_STATUS_DOWNING 0ULL
> > +#define FW_STATUS_RUNNING 2ULL
> > +#define CN9K_PEMX_PFX_CSX_PFCFGX(pem, pf, offset)
> ((0x8e0000008000 | (uint64_t)(pem) << 36 \
> > + | (pf) << 18 \
> > + | (((offset) >> 16) & 1)
> << 16 \
> > + | ((offset) >> 3) << 3) \
> > + + ((((offset) >> 2) & 1)
> << 2))
>
> I realise that this implementation mirrors that in octep_regs_cnxk_pf.h, but I
> do think it would be better rexpressed in terms of FIELD_PREP(),
> GETMASK_ULL, and BIT_ULL. With #defines so the masks (and bits are
> named).
>
> Also, as the above duplicates what is present in octep_regs_cnxk_pf.h, maybe
> it would be nice to share it somehow.
>
We can't have a common definition for different SoCs as configurations may vary from one SoC to another.
I can send out another version using bits handling macros suggested by you.
> > +
> > +/* Register defines for use with CN9K_PEMX_PFX_CSX_PFCFGX */ #define
> > +CN9K_PCIEEP_VSECST_CTL 0x4D0
> > +
> > #define CN93_PEM_BAR4_INDEX 7
> > #define CN93_PEM_BAR4_INDEX_SIZE 0x400000ULL
> > #define CN93_PEM_BAR4_INDEX_OFFSET (CN93_PEM_BAR4_INDEX *
> CN93_PEM_BAR4_INDEX_SIZE)
> > diff --git
> > a/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cnxk_pf.h
> > b/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cnxk_pf.h
> > index e637d7c8224d..a6b6c9f356de 100644
> > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cnxk_pf.h
> > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cnxk_pf.h
> > @@ -396,6 +396,7 @@
> > #define CNXK_SDP_EPF_OEI_RINT_DATA_BIT_MBOX BIT_ULL(0)
> > /* bit 1 for firmware heartbeat interrupt */
> > #define CNXK_SDP_EPF_OEI_RINT_DATA_BIT_HBEAT BIT_ULL(1)
> > +#define FW_STATUS_DOWNING 0ULL
> > #define FW_STATUS_RUNNING 2ULL
> > #define CNXK_PEMX_PFX_CSX_PFCFGX(pem, pf, offset) ({ typeof(offset)
> _off = (offset); \
> > ((0x8e0000008000 |
> \
> > --
> > 2.34.1
> >
> >
Powered by blists - more mailing lists