mlxsw: Add support for new reset flow
Subsystems (4)
Petr Machata
maintainercover mlxsw: Add support for new reset flow
Amit Cohen writes:
The driver resets the device during probe and during a devlink reload. The current reset method reloads the current firmware version or a pending one, if one was previously flashed using devlink. However, the reset does not take down the PCI link, preventing the PCI firmware from being upgraded, unless the system is rebooted.
To solve this problem, a new reset command was implemented in the firmware. Unlike the current command, after issuing the new command the device will not start the reset immediately, but only after the PCI link was disabled.
For Spectrum-4, the firmware will support the new reset flow, which includes PCI reset. Add support for the new flow and use it only after verifying it is supported by the current firmware version by querying the Management Capabilities Mask (MCAM) register.
Patch set overview:
Patch #1 fixes a wrong order of registers' definitions. Patches #2-#3 add the required registers and fields for new reset support.
Patches #4-#5 prepare reset code to be extended.
Patch #6 adds support for the new reset flow.
Amit Cohen (6):
mlxsw: reg: Move 'mpsc' definition in 'mlxsw_reg_infos'
mlxsw: reg: Add Management Capabilities Mask Register
mlxsw: Extend MRSR pack() function to support new commands
mlxsw: pci: Rename mlxsw_pci_sw_reset()
mlxsw: pci: Move software reset code to a separate function
mlxsw: pci: Add support for new reset flow
drivers/net/ethernet/mellanox/mlxsw/pci.c | 169 ++++++++++++++++++- drivers/net/ethernet/mellanox/mlxsw/pci_hw.h | 5 + drivers/net/ethernet/mellanox/mlxsw/reg.h | 90 +++++++++- 3 files changed, 254 insertions(+), 10 deletions(-)
Signature · 1 line
2.39.0
Petr Machata
maintainerPATCH 1/6 mlxsw: reg: Move 'mpsc' definition in 'mlxsw_reg_infos'
From: Amit Cohen <[email protected]>
The array 'mlxsw_reg_infos' is ordered by registers' IDs. The ID of MPSC register is 0x9080, so it should be after MCDA (register ID 0x9063) and not after MTUTC (register ID 0x9055). Note that the register's fields are defined in the correct place in the file, only the definition in 'mlxsw_reg_infos' is wrong. This issue was found while adding new register which supposed to be before mpsc.
--- drivers/net/ethernet/mellanox/mlxsw/reg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlxsw/reg.h +1 -1
@@ -12901,10 +12901,10 @@ static const struct mlxsw_reg_info *mlxsw_reg_infos[] = {MLXSW_REG (mcion),MLXSW_REG (mtpps),MLXSW_REG (mtutc), -MLXSW_REG (mpsc),MLXSW_REG (mcqi),MLXSW_REG (mcc),MLXSW_REG (mcda), +MLXSW_REG (mpsc),MLXSW_REG (mgpc),MLXSW_REG (mprs), MLXSW_REG(mogcr),
Signature · 1 line
2.39.0
Re: [PATCH net-next 1/6] mlxsw: reg: Move 'mpsc' definition in 'mlxsw_reg_infos'
Quoted 9 lines · Petr Machata
From: Amit Cohen <[email protected]>
The array '
mlxsw_reg_infos' is ordered by registers' IDs. The ID of MPSC register is0x9080, so it should be after MCDA (register ID0x9063) and not after MTUTC (register ID0x9055). Note that the register's fields are defined in the correct place in the file, only the definition in 'mlxsw_reg_infos' is wrong. This issue was found while adding new register which supposed to be before mpsc.
Quoted 4 lines
Signed-off-by Amit Cohen <[email protected]>Reviewed-by Ido Schimmel <[email protected]>Reviewed-by Petr Machata <[email protected]>Signed-off-by Petr Machata <[email protected]>
Petr Machata
maintainerPATCH 2/6 mlxsw: reg: Add Management Capabilities Mask Register
In reply to Petr MachataFrom: Amit Cohen <[email protected]>
MCAM register reports the device supported management features. Querying this register exposes if features are supported with the current firmware version in the current ASIC. Then, the drive can separate between different implementations dynamically.
MCAM register supports querying whether a new reset flow (which includes PCI reset) is supported or not. Add support for the register as preparation for support of the new reset flow.
Note that the access to the bits in the field 'mng_feature_cap_mask' is not same to other mask fields in other registers. In most of the cases bit #0 is the first one in the last dword, in MCAM register, bits #0-#31 are in the first dword and so on. Declare the mask field using bits arrays per dword to simplify the access.
--- drivers/net/ethernet/mellanox/mlxsw/reg.h | 74 +++++++++++++++++++++++ 1 file changed, 74 insertions(+)
drivers/net/ethernet/mellanox/mlxsw/reg.h +74 -0
@@ -10509,6 +10509,79 @@ static inline void mlxsw_reg_mcda_pack(char *payload, u32 update_handle,mlxsw_reg_mcda_data_set (payload, i, *(u32 *) &data[i *4 ]); } +/* MCAM - Management Capabilities Mask Register +* -------------------------------------------- +* Reports the device supported management features. +*/ +#define MLXSW_REG_MCAM_ID 0x907F +#define MLXSW_REG_MCAM_LEN 0x48 + +MLXSW_REG_DEFINE (mcam,MLXSW_REG_MCAM_ID ,MLXSW_REG_MCAM_LEN ); + +enum mlxsw_reg_mcam_feature_group { +/* Enhanced features. */ +MLXSW_REG_MCAM_FEATURE_GROUP_ENHANCED_FEATURES , +}; + +/* reg_mcam_feature_group +* Feature list mask index. +* Access: Index +*/ +MLXSW_ITEM32 (reg, mcam, feature_group,0x00 ,16 ,8 ); + +enum mlxsw_reg_mcam_mng_feature_cap_mask_bits { +/* If set, MRSR.command=6 is supported. */ +MLXSW_REG_MCAM_PCI_RESET =48 , +}; + +#define MLXSW_REG_BYTES_PER_DWORD 0x4 + +/* reg_mcam_mng_feature_cap_mask +* Supported port's enhanced features. +* Based on feature_group index. +* When bit is set, the feature is supported in the device. +* Access: RO +*/ +#define MLXSW_REG_MCAM_MNG_FEATURE_CAP_MASK_DWORD (_dw_num, _offset) \ + MLXSW_ITEM_BIT_ARRAY(reg, mcam, mng_feature_cap_mask_dw##_dw_num, \ + _offset, MLXSW_REG_BYTES_PER_DWORD, 1) + +/* The access to the bits in the field 'mng_feature_cap_mask' is not same to +* other mask fields in other registers. In most of the cases bit #0 is the +* first one in the last dword. In MCAM register, the first dword contains bits +* #0-#31 and so on, so the access to the bits is simpler using bit array per +* dword. Declare each dword of 'mng_feature_cap_mask' field separately. +*/ +MLXSW_REG_MCAM_MNG_FEATURE_CAP_MASK_DWORD (0 ,0x28 ); +MLXSW_REG_MCAM_MNG_FEATURE_CAP_MASK_DWORD (1 ,0x2C ); +MLXSW_REG_MCAM_MNG_FEATURE_CAP_MASK_DWORD (2 ,0x30 ); +MLXSW_REG_MCAM_MNG_FEATURE_CAP_MASK_DWORD (3 ,0x34 ); + +static inline void +mlxsw_reg_mcam_pack (char *payload,enum mlxsw_reg_mcam_feature_group feat_group) +{ +MLXSW_REG_ZERO (mcam, payload); +mlxsw_reg_mcam_feature_group_set (payload, feat_group); +} + +static inline void +mlxsw_reg_mcam_unpack (char *payload, +enum mlxsw_reg_mcam_mng_feature_cap_mask_bits bit, +bool *p_mng_feature_cap_val) +{ +int offset = bit % (MLXSW_REG_BYTES_PER_DWORD *BITS_PER_BYTE ); +int dword = bit / (MLXSW_REG_BYTES_PER_DWORD *BITS_PER_BYTE ); +u8 (*getters[])(const char *,u16 ) = { + mlxsw_reg_mcam_mng_feature_cap_mask_dw0_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw1_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw2_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw3_get, + }; + +if (!WARN_ON_ONCE (dword >=ARRAY_SIZE (getters))) + *p_mng_feature_cap_val = getters[dword](payload, offset); +} +/* MPSC - Monitoring Packet Sampling Configuration Register * -------------------------------------------------------- * MPSC Register is used to configure the Packet Sampling mechanism.
@@ -12904,6 +12977,7 @@ static const struct mlxsw_reg_info *mlxsw_reg_infos[] = {MLXSW_REG (mcqi),MLXSW_REG (mcc),MLXSW_REG (mcda), +MLXSW_REG (mcam),MLXSW_REG (mpsc),MLXSW_REG (mgpc), MLXSW_REG(mprs),
Signature · 1 line
2.39.0
Re: [PATCH net-next 2/6] mlxsw: reg: Add Management Capabilities Mask Register
Quoted 17 lines · Petr Machata
From: Amit Cohen <[email protected]>
MCAM register reports the device supported management features. Querying this register exposes if features are supported with the current firmware version in the current ASIC. Then, the drive can separate between different implementations dynamically.
MCAM register supports querying whether a new reset flow (which includes PCI reset) is supported or not. Add support for the register as preparation for support of the new reset flow.
Note that the access to the bits in the field '
mng_feature_cap_mask' is not same to other mask fields in other registers. In most of the cases bit #0 is the first one in the last dword, in MCAM register, bits #0-#31 are in the first dword and so on. Declare the mask field using bits arrays per dword to simplify the access.
Quoted 3 lines
Signed-off-by Amit Cohen <[email protected]>Reviewed-by Petr Machata <[email protected]>Signed-off-by Petr Machata <[email protected]>
Re: [PATCH net-next 2/6] mlxsw: reg: Add Management Capabilities Mask Register
In reply to Petr MachataQuoted 17 lines · Petr Machata
From: Amit Cohen <[email protected]>
MCAM register reports the device supported management features. Querying this register exposes if features are supported with the current firmware version in the current ASIC. Then, the drive can separate between different implementations dynamically.
MCAM register supports querying whether a new reset flow (which includes PCI reset) is supported or not. Add support for the register as preparation for support of the new reset flow.
Note that the access to the bits in the field '
mng_feature_cap_mask' is not same to other mask fields in other registers. In most of the cases bit #0 is the first one in the last dword, in MCAM register, bits #0-#31 are in the first dword and so on. Declare the mask field using bits arrays per dword to simplify the access.
Quoted 3 lines
Signed-off-by Amit Cohen <[email protected]>Reviewed-by Petr Machata <[email protected]>Signed-off-by Petr Machata <[email protected]>
I'm fine with this patch, and offered a Reviewed-by tag in another email. But when sending that I forgot the minor nit below.
Please regard it as informational only.
Quoted 8 lines
--- drivers/net/ethernet/mellanox/mlxsw/reg.h | 74 +++++++++++++++++++++++ 1 file changed, 74 insertions(+)drivers/net/ethernet/mellanox/mlxsw/reg.h +0
-0
...
drivers/net/ethernet/mellanox/mlxsw/reg.h static inline void mlxsw_reg_mcda_pack(char *payload, u32 update_handle, · line 10573
Quoted 7 lines
drivers/net/ethernet/mellanox/mlxsw/reg.h +7
-0+static inline void +mlxsw_reg_mcam_unpack (char *payload, +enum mlxsw_reg_mcam_mng_feature_cap_mask_bits bit, +bool *p_mng_feature_cap_val) +{ +int offset = bit % (MLXSW_REG_BYTES_PER_DWORD *BITS_PER_BYTE ); +int dword = bit / (MLXSW_REG_BYTES_PER_DWORD *BITS_PER_BYTE );
nit: checkpatch seems mildly upset that there is no blank line here.
Quoted 6 lines
drivers/net/ethernet/mellanox/mlxsw/reg.h +6
-0+u8 (*getters[])(const char *,u16 ) = { + mlxsw_reg_mcam_mng_feature_cap_mask_dw0_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw1_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw2_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw3_get, + };
Petr Machata
maintainerRe: [PATCH net-next 2/6] mlxsw: reg: Add Management Capabilities Mask Register
Quoted 37 lines · Simon Horman
Quoted 17 lines · Petr Machata
From: Amit Cohen <[email protected]>
MCAM register reports the device supported management features. Querying this register exposes if features are supported with the current firmware version in the current ASIC. Then, the drive can separate between different implementations dynamically.
MCAM register supports querying whether a new reset flow (which includes PCI reset) is supported or not. Add support for the register as preparation for support of the new reset flow.
Note that the access to the bits in the field '
mng_feature_cap_mask' is not same to other mask fields in other registers. In most of the cases bit #0 is the first one in the last dword, in MCAM register, bits #0-#31 are in the first dword and so on. Declare the mask field using bits arrays per dword to simplify the access.Quoted 3 lines
Signed-off-by Amit Cohen <[email protected]>Reviewed-by Petr Machata <[email protected]>Signed-off-by Petr Machata <[email protected]>I'm fine with this patch, and offered a Reviewed-by tag in another email. But when sending that I forgot the minor nit below.
Please regard it as informational only.Quoted 8 lines
--- drivers/net/ethernet/mellanox/mlxsw/reg.h | 74 +++++++++++++++++++++++ 1 file changed, 74 insertions(+)drivers/net/ethernet/mellanox/mlxsw/reg.h +0
-0...
Quoted 9 lines
drivers/net/ethernet/mellanox/mlxsw/reg.h static inline void mlxsw_reg_mcda_pack(char *payload, u32 update_handle, · line 10573
Quoted 7 lines
drivers/net/ethernet/mellanox/mlxsw/reg.h +7
-0+static inline void +mlxsw_reg_mcam_unpack (char *payload, +enum mlxsw_reg_mcam_mng_feature_cap_mask_bits bit, +bool *p_mng_feature_cap_val) +{ +int offset = bit % (MLXSW_REG_BYTES_PER_DWORD *BITS_PER_BYTE ); +int dword = bit / (MLXSW_REG_BYTES_PER_DWORD *BITS_PER_BYTE );nit: checkpatch seems mildly upset that there is no blank line here.
Yes, thanks for pointing this out. I saw that, too. The complaint is that there should be a blank line after the declaration block, but the next line is still a part of the declaration block.
Quoted 6 lines
Quoted 6 lines
drivers/net/ethernet/mellanox/mlxsw/reg.h +6
-0+u8 (*getters[])(const char *,u16 ) = { + mlxsw_reg_mcam_mng_feature_cap_mask_dw0_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw1_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw2_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw3_get, + };
Re: [PATCH net-next 2/6] mlxsw: reg: Add Management Capabilities Mask Register
Quoted 50 lines · Petr Machata
Quoted 37 lines · Simon Horman
Quoted 17 lines · Petr Machata
From: Amit Cohen <[email protected]>
MCAM register reports the device supported management features. Querying this register exposes if features are supported with the current firmware version in the current ASIC. Then, the drive can separate between different implementations dynamically.
MCAM register supports querying whether a new reset flow (which includes PCI reset) is supported or not. Add support for the register as preparation for support of the new reset flow.
Note that the access to the bits in the field '
mng_feature_cap_mask' is not same to other mask fields in other registers. In most of the cases bit #0 is the first one in the last dword, in MCAM register, bits #0-#31 are in the first dword and so on. Declare the mask field using bits arrays per dword to simplify the access.Quoted 3 lines
Signed-off-by Amit Cohen <[email protected]>Reviewed-by Petr Machata <[email protected]>Signed-off-by Petr Machata <[email protected]>I'm fine with this patch, and offered a Reviewed-by tag in another email. But when sending that I forgot the minor nit below.
Please regard it as informational only.Quoted 8 lines
--- drivers/net/ethernet/mellanox/mlxsw/reg.h | 74 +++++++++++++++++++++++ 1 file changed, 74 insertions(+)drivers/net/ethernet/mellanox/mlxsw/reg.h +0
-0...
Quoted 9 lines
drivers/net/ethernet/mellanox/mlxsw/reg.h static inline void mlxsw_reg_mcda_pack(char *payload, u32 update_handle, · line 10573
Quoted 7 lines
drivers/net/ethernet/mellanox/mlxsw/reg.h +7
-0+static inline void +mlxsw_reg_mcam_unpack (char *payload, +enum mlxsw_reg_mcam_mng_feature_cap_mask_bits bit, +bool *p_mng_feature_cap_val) +{ +int offset = bit % (MLXSW_REG_BYTES_PER_DWORD *BITS_PER_BYTE ); +int dword = bit / (MLXSW_REG_BYTES_PER_DWORD *BITS_PER_BYTE );nit: checkpatch seems mildly upset that there is no blank line here.
Quoted 3 lines
Yes, thanks for pointing this out. I saw that, too. The complaint is that there should be a blank line after the declaration block, but the next line is still a part of the declaration block.
Yes, right you are.
Sorry for missing that.
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
drivers/net/ethernet/mellanox/mlxsw/reg.h +6
-0+u8 (*getters[])(const char *,u16 ) = { + mlxsw_reg_mcam_mng_feature_cap_mask_dw0_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw1_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw2_get, + mlxsw_reg_mcam_mng_feature_cap_mask_dw3_get, + };
Petr Machata
maintainerPATCH 3/6 mlxsw: Extend MRSR pack() function to support new commands
In reply to Petr MachataFrom: Amit Cohen <[email protected]>
Currently mlxsw_reg_mrsr_pack() always sets 'command=1'. As preparation for support of new reset flow, pass the command as an argument to the function and add an enum for this field.
For now, always pass 'command=1' to the pack() function.
--- drivers/net/ethernet/mellanox/mlxsw/pci.c | 2 +- drivers/net/ethernet/mellanox/mlxsw/reg.h | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/pci.c +1 -1
@@ -1474,7 +1474,7 @@ static int mlxsw_pci_sw_reset(struct mlxsw_pci *mlxsw_pci,return err; } -mlxsw_reg_mrsr_pack (mrsr_pl); +mlxsw_reg_mrsr_pack (mrsr_pl,MLXSW_REG_MRSR_COMMAND_SOFTWARE_RESET ); err =mlxsw_reg_write (mlxsw_pci->core,MLXSW_REG (mrsr), mrsr_pl);if (err)return err;
drivers/net/ethernet/mellanox/mlxsw/reg.h +12 -2
@@ -10070,6 +10070,15 @@ mlxsw_reg_mgir_unpack(char *payload, u32 *hw_rev, char *fw_info_psid,MLXSW_REG_DEFINE (mrsr,MLXSW_REG_MRSR_ID ,MLXSW_REG_MRSR_LEN ); +enum mlxsw_reg_mrsr_command { +/* Switch soft reset, does not reset PCI firmware. */ +MLXSW_REG_MRSR_COMMAND_SOFTWARE_RESET =1 , +/* Reset will be done when PCI link will be disabled. +* This command will reset PCI firmware also. +*/ +MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE =6 , +}; +/* reg_mrsr_command * Reset/shutdown command * 0 - do nothing
@@ -10078,10 +10087,11 @@ MLXSW_REG_DEFINE(mrsr, MLXSW_REG_MRSR_ID, MLXSW_REG_MRSR_LEN);*/ MLXSW_ITEM32 (reg, mrsr, command,0x00 ,0 ,4 ); -static inline void mlxsw_reg_mrsr_pack (char *payload) +static inline void mlxsw_reg_mrsr_pack (char *payload, +enum mlxsw_reg_mrsr_command command) {MLXSW_REG_ZERO (mrsr, payload); -mlxsw_reg_mrsr_command_set (payload,1 ); +mlxsw_reg_mrsr_command_set (payload, command); }/* MLCR - Management LED Control Register
Signature · 1 line
2.39.0
Re: [PATCH net-next 3/6] mlxsw: Extend MRSR pack() function to support new commands
Quoted 6 lines · Petr Machata
From: Amit Cohen <[email protected]>
Currently
mlxsw_reg_mrsr_pack()always sets 'command=1'. As preparation for support of new reset flow, pass the command as an argument to the function and add an enum for this field.
Quoted 6 lines
For now, always pass 'command=1' to the
pack()function.Signed-off-by Amit Cohen <[email protected]>Reviewed-by Petr Machata <[email protected]>Reviewed-by Ido Schimmel <[email protected]>Signed-off-by Petr Machata <[email protected]>
Petr Machata
maintainerPATCH 4/6 mlxsw: pci: Rename mlxsw_pci_sw_reset()
In reply to Petr MachataFrom: Amit Cohen <[email protected]>
In the next patches, mlxsw_pci_sw_reset() will be extended to support more reset types and will not necessarily issue a software reset. Rename the function to reflect that.
--- drivers/net/ethernet/mellanox/mlxsw/pci.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/pci.c +5 -5
@@ -1459,8 +1459,8 @@ static int mlxsw_pci_sys_ready_wait(struct mlxsw_pci *mlxsw_pci,return -EBUSY ; } -static int mlxsw_pci_sw_reset (struct mlxsw_pci *mlxsw_pci, -const struct pci_device_id *id) +static int +mlxsw_pci_reset (struct mlxsw_pci *mlxsw_pci,const struct pci_device_id *id) {struct pci_dev *pdev = mlxsw_pci->pdev;char mrsr_pl[MLXSW_REG_MRSR_LEN ];
@@ -1520,9 +1520,9 @@ static int mlxsw_pci_init(void *bus_priv, struct mlxsw_core *mlxsw_core,if (!mbox)return -ENOMEM ; - err =mlxsw_pci_sw_reset (mlxsw_pci, mlxsw_pci->id); + err =mlxsw_pci_reset (mlxsw_pci, mlxsw_pci->id);if (err) - goto err_sw_reset; + goto err_reset; err =mlxsw_pci_alloc_irq_vectors (mlxsw_pci);if (err <0 ) {
@@ -1654,7 +1654,7 @@ static int mlxsw_pci_init(void *bus_priv, struct mlxsw_core *mlxsw_core, err_query_fw:mlxsw_pci_free_irq_vectors (mlxsw_pci); err_alloc_irq: -err_sw_reset: +err_reset: mbox_put:mlxsw_cmd_mbox_free (mbox);return err;
Signature · 1 line
2.39.0
Re: [PATCH net-next 4/6] mlxsw: pci: Rename mlxsw_pci_sw_reset()
Quoted 9 lines · Petr Machata
From: Amit Cohen <[email protected]>
In the next patches,
mlxsw_pci_sw_reset()will be extended to support more reset types and will not necessarily issue a software reset. Rename the function to reflect that.Signed-off-by Amit Cohen <[email protected]>Reviewed-by Ido Schimmel <[email protected]>Signed-off-by Petr Machata <[email protected]>
Petr Machata
maintainerPATCH 5/6 mlxsw: pci: Move software reset code to a separate function
In reply to Petr MachataFrom: Amit Cohen <[email protected]>
In general, the existing flow of software reset in the driver is:
1. Wait for system ready status.
2. Send MRSR command, to start the reset.
3. Wait for system ready status.
This flow will be extended once a new reset command is supported. As a preparation, move step #2 to a separate function.
--- drivers/net/ethernet/mellanox/mlxsw/pci.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/pci.c +11 -5
@@ -1459,11 +1459,18 @@ static int mlxsw_pci_sys_ready_wait(struct mlxsw_pci *mlxsw_pci,return -EBUSY ; } -static int -mlxsw_pci_reset (struct mlxsw_pci *mlxsw_pci,const struct pci_device_id *id) +static int mlxsw_pci_reset_sw (struct mlxsw_pci *mlxsw_pci) { -struct pci_dev *pdev = mlxsw_pci->pdev;char mrsr_pl[MLXSW_REG_MRSR_LEN ]; + +mlxsw_reg_mrsr_pack (mrsr_pl,MLXSW_REG_MRSR_COMMAND_SOFTWARE_RESET ); +return mlxsw_reg_write (mlxsw_pci->core,MLXSW_REG (mrsr), mrsr_pl); +} + +static int +mlxsw_pci_reset (struct mlxsw_pci *mlxsw_pci,const struct pci_device_id *id) +{ +struct pci_dev *pdev = mlxsw_pci->pdev;u32 sys_status;int err;
@@ -1474,8 +1481,7 @@ mlxsw_pci_reset(struct mlxsw_pci *mlxsw_pci, const struct pci_device_id *id)return err; } -mlxsw_reg_mrsr_pack (mrsr_pl,MLXSW_REG_MRSR_COMMAND_SOFTWARE_RESET ); - err =mlxsw_reg_write (mlxsw_pci->core,MLXSW_REG (mrsr), mrsr_pl); + err =mlxsw_pci_reset_sw (mlxsw_pci);if (err)return err;
Signature · 1 line
2.39.0
Re: [PATCH net-next 5/6] mlxsw: pci: Move software reset code to a separate function
Quoted 7 lines · Petr Machata
From: Amit Cohen <[email protected]>
In general, the existing flow of software reset in the driver is:
1. Wait for system ready status.
2. Send MRSR command, to start the reset.
3. Wait for system ready status.
Quoted 7 lines
This flow will be extended once a new reset command is supported. As a preparation, move step #2 to a separate function.
Signed-off-by Amit Cohen <[email protected]>Reviewed-by Petr Machata <[email protected]>Reviewed-by Ido Schimmel <[email protected]>Signed-off-by Petr Machata <[email protected]>
Petr Machata
maintainerPATCH 6/6 mlxsw: pci: Add support for new reset flow
In reply to Petr MachataFrom: Amit Cohen <[email protected]>
The driver resets the device during probe and during a devlink reload. The current reset method reloads the current firmware version or a pending one, if one was previously flashed using devlink. However, the reset does not take down the PCI link, preventing the PCI firmware from being upgraded, unless the system is rebooted.
To solve this problem, a new reset command (6) was implemented in the firmware. Unlike the current command (1), after issuing the new command the device will not start the reset immediately, but only after the PCI link was disabled. The driver is expected to wait for 500ms before re-enabling the link to give the firmware enough time to start the reset.
Implement the new reset method and use it only after verifying it is supported by the current firmware version by querying the Management Capabilities Mask (MCAM) register. Consider the PCI firmware to be operational either after waiting for a predefined time of 2000ms or after reading an active link status when "Data Link Layer Link Active Reporting" is supported. For good measures, make sure the device ID can be read from the configuration space of the device.
Once the PCI firmware is operational, go back to the regular reset flow and wait for the entire device to become ready. That is, repeatedly read the "system_status" register from the BAR until a value of "FW_READY" (0x5E) appears.
--- drivers/net/ethernet/mellanox/mlxsw/pci.c | 151 ++++++++++++++++++- drivers/net/ethernet/mellanox/mlxsw/pci_hw.h | 5 + 2 files changed, 155 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlxsw/pci.c +150 -1
@@ -1459,6 +1459,137 @@ static int mlxsw_pci_sys_ready_wait(struct mlxsw_pci *mlxsw_pci,return -EBUSY ; } +static int mlxsw_pci_link_active_wait (struct pci_dev *pdev) +{ +unsigned long end; +u16 lnksta; +int err; + + end = jiffies +msecs_to_jiffies (MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); +do { +msleep (MLXSW_PCI_TOGGLE_WAIT_MSECS ); + err =pcie_capability_read_word (pdev,PCI_EXP_LNKSTA , &lnksta); +if (err) +return pcibios_err_to_errno (err); + +if (lnksta &PCI_EXP_LNKSTA_DLLLA ) +return 0 ; + }while (time_before (jiffies, end)); + +pci_err (pdev,"PCI link not ready (0x%04x) after %d ms\n" , lnksta, +MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); + +return -ETIMEDOUT ; +} + +static int mlxsw_pci_link_active_check (struct pci_dev *pdev) +{ +u32 lnkcap; +int err; + + err =pcie_capability_read_dword (pdev,PCI_EXP_LNKCAP , &lnkcap); +if (err) + goto out; + +if (lnkcap &PCI_EXP_LNKCAP_DLLLARC ) +return mlxsw_pci_link_active_wait (pdev); + +/* In case the device does not support "Data Link Layer Link Active +* Reporting", simply wait for a predefined time for the device to +* become active. +*/ +pci_dbg (pdev,"No PCI link reporting capability (0x%08x)\n" , lnkcap); + +out: +/* Sleep before handling the rest of the flow and accessing to PCI. */ +msleep (MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); +return pcibios_err_to_errno (err); +} + +static int mlxsw_pci_link_toggle (struct pci_dev *pdev) +{ +int err; + +/* Disable the link. */ + err =pcie_capability_set_word (pdev,PCI_EXP_LNKCTL ,PCI_EXP_LNKCTL_LD ); +if (err) +return pcibios_err_to_errno (err); + +/* Sleep to give firmware enough time to start the reset. */ +msleep (MLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS ); + +/* Enable the link. */ + err =pcie_capability_clear_word (pdev,PCI_EXP_LNKCTL , +PCI_EXP_LNKCTL_LD ); +if (err) +return pcibios_err_to_errno (err); + +/* Wait for link active. */ +return mlxsw_pci_link_active_check (pdev); +} + +static int mlxsw_pci_device_id_read (struct pci_dev *pdev,u16 exp_dev_id) +{ +unsigned long end; +u16 dev_id; +int err; + + end = jiffies +msecs_to_jiffies (MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); +do { +msleep (MLXSW_PCI_TOGGLE_WAIT_MSECS ); + +/* Expect to get the correct PCI device ID as first indication +* that the ASIC is available. +*/ + err =pci_read_config_word (pdev,PCI_DEVICE_ID , &dev_id); +if (err) +return pcibios_err_to_errno (err); + +if (dev_id == exp_dev_id) +return 0 ; + }while (time_before (jiffies, end)); + +pci_err (pdev,"PCI device ID is not as expected after %d ms\n" , +MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); + +return -ETIMEDOUT ; +} + +static int mlxsw_pci_reset_at_pci_disable (struct mlxsw_pci *mlxsw_pci) +{ +struct pci_bus *bridge_bus = mlxsw_pci->pdev->bus; +struct pci_dev *bridge_pdev = bridge_bus->self; +struct pci_dev *pdev = mlxsw_pci->pdev; +char mrsr_pl[MLXSW_REG_MRSR_LEN ]; +u16 dev_id = pdev->device; +int err; + +mlxsw_reg_mrsr_pack (mrsr_pl, +MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE ); + err =mlxsw_reg_write (mlxsw_pci->core,MLXSW_REG (mrsr), mrsr_pl); +if (err) +return err; + +/* Save the PCI configuration space so that we will be able to restore +* it after the firmware was reset. +*/ +pci_save_state (pdev); +pci_cfg_access_lock (pdev); + + err =mlxsw_pci_link_toggle (bridge_pdev); +if (err) { +pci_err (bridge_pdev,"Failed to toggle PCI link\n" ); + goto restore; + } + + err =mlxsw_pci_device_id_read (pdev, dev_id); + +restore: +pci_cfg_access_unlock (pdev); +pci_restore_state (pdev); +return err; +} +static int mlxsw_pci_reset_sw (struct mlxsw_pci *mlxsw_pci) {char mrsr_pl[MLXSW_REG_MRSR_LEN ];
@@ -1471,6 +1602,8 @@ static int mlxsw_pci_reset(struct mlxsw_pci *mlxsw_pci, const struct pci_device_id *id) { struct pci_dev *pdev = mlxsw_pci->pdev; +char mcam_pl[MLXSW_REG_MCAM_LEN ]; +bool pci_reset_supported;u32 sys_status;int err;
@@ -1481,7 +1614,23 @@ mlxsw_pci_reset(struct mlxsw_pci *mlxsw_pci, const struct pci_device_id *id)return err; } - err =mlxsw_pci_reset_sw (mlxsw_pci); +mlxsw_reg_mcam_pack (mcam_pl, +MLXSW_REG_MCAM_FEATURE_GROUP_ENHANCED_FEATURES ); + err =mlxsw_reg_query (mlxsw_pci->core,MLXSW_REG (mcam), mcam_pl); +if (err) +return err; + +mlxsw_reg_mcam_unpack (mcam_pl,MLXSW_REG_MCAM_PCI_RESET , + &pci_reset_supported); + +if (pci_reset_supported) { +pci_dbg (pdev,"Starting PCI reset flow\n" ); + err =mlxsw_pci_reset_at_pci_disable (mlxsw_pci); + }else { +pci_dbg (pdev,"Starting software reset flow\n" ); + err =mlxsw_pci_reset_sw (mlxsw_pci); + } +if (err)return err;
drivers/net/ethernet/mellanox/mlxsw/pci_hw.h +5 -0
@@ -27,6 +27,11 @@#define MLXSW_PCI_SW_RESET_TIMEOUT_MSECS 900000#define MLXSW_PCI_SW_RESET_WAIT_MSECS 200 + +#define MLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS 500 +#define MLXSW_PCI_TOGGLE_WAIT_MSECS 20 +#define MLXSW_PCI_TOGGLE_TIMEOUT_MSECS 2000 +#define MLXSW_PCI_FW_READY 0xA1844#define MLXSW_PCI_FW_READY_MASK 0xFFFF#define MLXSW_PCI_FW_READY_MAGIC 0x5E
Signature · 1 line
2.39.0
Petr Machata
maintainerRe: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow
Quoted 22 lines · Petr Machata
From: Amit Cohen <[email protected]>
The driver resets the device during probe and during a devlink reload. The current reset method reloads the current firmware version or a pending one, if one was previously flashed using devlink. However, the reset does not take down the PCI link, preventing the PCI firmware from being upgraded, unless the system is rebooted.
To solve this problem, a new reset command (6) was implemented in the firmware. Unlike the current command (1), after issuing the new command the device will not start the reset immediately, but only after the PCI link was disabled. The driver is expected to wait for 500ms before re-enabling the link to give the firmware enough time to start the reset.
Implement the new reset method and use it only after verifying it is supported by the current firmware version by querying the Management Capabilities Mask (MCAM) register. Consider the PCI firmware to be operational either after waiting for a predefined time of 2000ms or after reading an active link status when "Data Link Layer Link Active Reporting" is supported. For good measures, make sure the device ID can be read from the configuration space of the device.
Quoted 8 lines
Once the PCI firmware is operational, go back to the regular reset flow and wait for the entire device to become ready. That is, repeatedly read the "
system_status" register from the BAR until a value of "FW_READY" (0x5E) appears.Signed-off-by Amit Cohen <[email protected]>Reviewed-by Ido Schimmel <[email protected]>Signed-off-by Petr Machata <[email protected]>
What I forgot to do was to CC PCI maintainer / subsystem. None of us in the mlxsw team is well versed in PCI odds and ends, and we would appreciate a sanity check of this code.
Quoted 197 lines
--- drivers/net/ethernet/mellanox/mlxsw/pci.c | 151 ++++++++++++++++++- drivers/net/ethernet/mellanox/mlxsw/pci_hw.h | 5 + 2 files changed, 155 insertions(+), 1 deletion(-)drivers/net/ethernet/mellanox/mlxsw/pci.c +150
-1@@ -1459,6 +1459,137 @@ static int mlxsw_pci_sys_ready_wait(struct mlxsw_pci *mlxsw_pci,return -EBUSY ; } +static int mlxsw_pci_link_active_wait (struct pci_dev *pdev) +{ +unsigned long end; +u16 lnksta; +int err; + + end = jiffies +msecs_to_jiffies (MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); +do { +msleep (MLXSW_PCI_TOGGLE_WAIT_MSECS ); + err =pcie_capability_read_word (pdev,PCI_EXP_LNKSTA , &lnksta); +if (err) +return pcibios_err_to_errno (err); + +if (lnksta &PCI_EXP_LNKSTA_DLLLA ) +return 0 ; + }while (time_before (jiffies, end)); + +pci_err (pdev,"PCI link not ready (0x%04x) after %d ms\n" , lnksta, +MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); + +return -ETIMEDOUT ; +} + +static int mlxsw_pci_link_active_check (struct pci_dev *pdev) +{ +u32 lnkcap; +int err; + + err =pcie_capability_read_dword (pdev,PCI_EXP_LNKCAP , &lnkcap); +if (err) + goto out; + +if (lnkcap &PCI_EXP_LNKCAP_DLLLARC ) +return mlxsw_pci_link_active_wait (pdev); + +/* In case the device does not support "Data Link Layer Link Active +* Reporting", simply wait for a predefined time for the device to +* become active. +*/ +pci_dbg (pdev,"No PCI link reporting capability (0x%08x)\n" , lnkcap); + +out: +/* Sleep before handling the rest of the flow and accessing to PCI. */ +msleep (MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); +return pcibios_err_to_errno (err); +} + +static int mlxsw_pci_link_toggle (struct pci_dev *pdev) +{ +int err; + +/* Disable the link. */ + err =pcie_capability_set_word (pdev,PCI_EXP_LNKCTL ,PCI_EXP_LNKCTL_LD ); +if (err) +return pcibios_err_to_errno (err); + +/* Sleep to give firmware enough time to start the reset. */ +msleep (MLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS ); + +/* Enable the link. */ + err =pcie_capability_clear_word (pdev,PCI_EXP_LNKCTL , +PCI_EXP_LNKCTL_LD ); +if (err) +return pcibios_err_to_errno (err); + +/* Wait for link active. */ +return mlxsw_pci_link_active_check (pdev); +} + +static int mlxsw_pci_device_id_read (struct pci_dev *pdev,u16 exp_dev_id) +{ +unsigned long end; +u16 dev_id; +int err; + + end = jiffies +msecs_to_jiffies (MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); +do { +msleep (MLXSW_PCI_TOGGLE_WAIT_MSECS ); + +/* Expect to get the correct PCI device ID as first indication +* that the ASIC is available. +*/ + err =pci_read_config_word (pdev,PCI_DEVICE_ID , &dev_id); +if (err) +return pcibios_err_to_errno (err); + +if (dev_id == exp_dev_id) +return 0 ; + }while (time_before (jiffies, end)); + +pci_err (pdev,"PCI device ID is not as expected after %d ms\n" , +MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); + +return -ETIMEDOUT ; +} + +static int mlxsw_pci_reset_at_pci_disable (struct mlxsw_pci *mlxsw_pci) +{ +struct pci_bus *bridge_bus = mlxsw_pci->pdev->bus; +struct pci_dev *bridge_pdev = bridge_bus->self; +struct pci_dev *pdev = mlxsw_pci->pdev; +char mrsr_pl[MLXSW_REG_MRSR_LEN ]; +u16 dev_id = pdev->device; +int err; + +mlxsw_reg_mrsr_pack (mrsr_pl, +MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE ); + err =mlxsw_reg_write (mlxsw_pci->core,MLXSW_REG (mrsr), mrsr_pl); +if (err) +return err; + +/* Save the PCI configuration space so that we will be able to restore +* it after the firmware was reset. +*/ +pci_save_state (pdev); +pci_cfg_access_lock (pdev); + + err =mlxsw_pci_link_toggle (bridge_pdev); +if (err) { +pci_err (bridge_pdev,"Failed to toggle PCI link\n" ); + goto restore; + } + + err =mlxsw_pci_device_id_read (pdev, dev_id); + +restore: +pci_cfg_access_unlock (pdev); +pci_restore_state (pdev); +return err; +} +static int mlxsw_pci_reset_sw (struct mlxsw_pci *mlxsw_pci) {char mrsr_pl[MLXSW_REG_MRSR_LEN ];@@ -1471,6 +1602,8 @@ static int mlxsw_pci_reset(struct mlxsw_pci *mlxsw_pci, const struct pci_device_id *id) { struct pci_dev *pdev = mlxsw_pci->pdev; +char mcam_pl[MLXSW_REG_MCAM_LEN ]; +bool pci_reset_supported;u32 sys_status;int err;@@ -1481,7 +1614,23 @@ mlxsw_pci_reset(struct mlxsw_pci *mlxsw_pci, const struct pci_device_id *id)return err; } - err =mlxsw_pci_reset_sw (mlxsw_pci); +mlxsw_reg_mcam_pack (mcam_pl, +MLXSW_REG_MCAM_FEATURE_GROUP_ENHANCED_FEATURES ); + err =mlxsw_reg_query (mlxsw_pci->core,MLXSW_REG (mcam), mcam_pl); +if (err) +return err; + +mlxsw_reg_mcam_unpack (mcam_pl,MLXSW_REG_MCAM_PCI_RESET , + &pci_reset_supported); + +if (pci_reset_supported) { +pci_dbg (pdev,"Starting PCI reset flow\n" ); + err =mlxsw_pci_reset_at_pci_disable (mlxsw_pci); + }else { +pci_dbg (pdev,"Starting software reset flow\n" ); + err =mlxsw_pci_reset_sw (mlxsw_pci); + } +if (err)return err;drivers/net/ethernet/mellanox/mlxsw/pci_hw.h +5
-0@@ -27,6 +27,11 @@#define MLXSW_PCI_SW_RESET_TIMEOUT_MSECS 900000#define MLXSW_PCI_SW_RESET_WAIT_MSECS 200 + +#define MLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS 500 +#define MLXSW_PCI_TOGGLE_WAIT_MSECS 20 +#define MLXSW_PCI_TOGGLE_TIMEOUT_MSECS 2000 +#define MLXSW_PCI_FW_READY 0xA1844#define MLXSW_PCI_FW_READY_MASK 0xFFFF#define MLXSW_PCI_FW_READY_MAGIC 0x5E
Re: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow
In reply to Petr MachataHi Petr, thanks for pointing me here.
Quoted 7 lines · Petr Machata
From: Amit Cohen <[email protected]>
The driver resets the device during probe and during a devlink reload. The current reset method reloads the current firmware version or a pending one, if one was previously flashed using devlink. However, the reset does not take down the PCI link, preventing the PCI firmware from being upgraded, unless the system is rebooted.
Just to make sure I understand this correctly, the above sounds like "firmware" includes two parts that have different rules for loading:
- Current reset method is completely mlxsw-specific and resets the mlxsw core but not the PCIe interface; this loads only firmware part A
- A PCIe reset resets both the mlxsw core and the PCIe interface; this loads both firmware part A and part B
Quoted 5 lines
To solve this problem, a new reset command (6) was implemented in the firmware. Unlike the current command (1), after issuing the new command the device will not start the reset immediately, but only after the PCI link was disabled. The driver is expected to wait for 500ms before re-enabling the link to give the firmware enough time to start the reset.
I guess the idea here is that the mlxsw driver:
- Tells the firmware we're going to reset
(MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE)
- Saves PCI config state
- Disables the link (mlxsw_pci_link_toggle()), which causes a PCIe hot reset
- The firmware notices the link disable and starts its own internal reset
- The mlxsw driver waits 500ms
(MLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS)
- Enables link and waits for it to be active
(mlxsw_pci_link_active_check()
- Waits for device to be ready again (mlxsw_pci_device_id_read())
So the first question is why you don't simply use pci_reset_function(), since it is supposed to cause a reset and do all the necessary waiting for the device to be ready. This is quite complicated to do correctly; in fact, we still discover issues there regularly. There are many special cases in PCIe r6.0, sec 6.6.1, and it would be much better if we can avoid trying to handle them all in individual drivers.
Of course, pci_reset_function() does *not* include details like MLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS.
I assume that flashing the firmware to the device followed by a power cycle (without ever doing MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE) would load the new firmware everywhere. Can we not do the same with a PCIe reset?
Quoted 216 lines
Implement the new reset method and use it only after verifying it is supported by the current firmware version by querying the Management Capabilities Mask (MCAM) register. Consider the PCI firmware to be operational either after waiting for a predefined time of 2000ms or after reading an active link status when "Data Link Layer Link Active Reporting" is supported. For good measures, make sure the device ID can be read from the configuration space of the device.
Once the PCI firmware is operational, go back to the regular reset flow and wait for the entire device to become ready. That is, repeatedly read the "
system_status" register from the BAR until a value of "FW_READY" (0x5E) appears.Signed-off-by Amit Cohen <[email protected]>Reviewed-by Ido Schimmel <[email protected]>Signed-off-by Petr Machata <[email protected]>--- drivers/net/ethernet/mellanox/mlxsw/pci.c | 151 ++++++++++++++++++- drivers/net/ethernet/mellanox/mlxsw/pci_hw.h | 5 + 2 files changed, 155 insertions(+), 1 deletion(-)drivers/net/ethernet/mellanox/mlxsw/pci.c +150
-1@@ -1459,6 +1459,137 @@ static int mlxsw_pci_sys_ready_wait(struct mlxsw_pci *mlxsw_pci,return -EBUSY ; } +static int mlxsw_pci_link_active_wait (struct pci_dev *pdev) +{ +unsigned long end; +u16 lnksta; +int err; + + end = jiffies +msecs_to_jiffies (MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); +do { +msleep (MLXSW_PCI_TOGGLE_WAIT_MSECS ); + err =pcie_capability_read_word (pdev,PCI_EXP_LNKSTA , &lnksta); +if (err) +return pcibios_err_to_errno (err); + +if (lnksta &PCI_EXP_LNKSTA_DLLLA ) +return 0 ; + }while (time_before (jiffies, end)); + +pci_err (pdev,"PCI link not ready (0x%04x) after %d ms\n" , lnksta, +MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); + +return -ETIMEDOUT ; +} + +static int mlxsw_pci_link_active_check (struct pci_dev *pdev) +{ +u32 lnkcap; +int err; + + err =pcie_capability_read_dword (pdev,PCI_EXP_LNKCAP , &lnkcap); +if (err) + goto out; + +if (lnkcap &PCI_EXP_LNKCAP_DLLLARC ) +return mlxsw_pci_link_active_wait (pdev); + +/* In case the device does not support "Data Link Layer Link Active +* Reporting", simply wait for a predefined time for the device to +* become active. +*/ +pci_dbg (pdev,"No PCI link reporting capability (0x%08x)\n" , lnkcap); + +out: +/* Sleep before handling the rest of the flow and accessing to PCI. */ +msleep (MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); +return pcibios_err_to_errno (err); +} + +static int mlxsw_pci_link_toggle (struct pci_dev *pdev) +{ +int err; + +/* Disable the link. */ + err =pcie_capability_set_word (pdev,PCI_EXP_LNKCTL ,PCI_EXP_LNKCTL_LD ); +if (err) +return pcibios_err_to_errno (err); + +/* Sleep to give firmware enough time to start the reset. */ +msleep (MLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS ); + +/* Enable the link. */ + err =pcie_capability_clear_word (pdev,PCI_EXP_LNKCTL , +PCI_EXP_LNKCTL_LD ); +if (err) +return pcibios_err_to_errno (err); + +/* Wait for link active. */ +return mlxsw_pci_link_active_check (pdev); +} + +static int mlxsw_pci_device_id_read (struct pci_dev *pdev,u16 exp_dev_id) +{ +unsigned long end; +u16 dev_id; +int err; + + end = jiffies +msecs_to_jiffies (MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); +do { +msleep (MLXSW_PCI_TOGGLE_WAIT_MSECS ); + +/* Expect to get the correct PCI device ID as first indication +* that the ASIC is available. +*/ + err =pci_read_config_word (pdev,PCI_DEVICE_ID , &dev_id); +if (err) +return pcibios_err_to_errno (err); + +if (dev_id == exp_dev_id) +return 0 ; + }while (time_before (jiffies, end)); + +pci_err (pdev,"PCI device ID is not as expected after %d ms\n" , +MLXSW_PCI_TOGGLE_TIMEOUT_MSECS ); + +return -ETIMEDOUT ; +} + +static int mlxsw_pci_reset_at_pci_disable (struct mlxsw_pci *mlxsw_pci) +{ +struct pci_bus *bridge_bus = mlxsw_pci->pdev->bus; +struct pci_dev *bridge_pdev = bridge_bus->self; +struct pci_dev *pdev = mlxsw_pci->pdev; +char mrsr_pl[MLXSW_REG_MRSR_LEN ]; +u16 dev_id = pdev->device; +int err; + +mlxsw_reg_mrsr_pack (mrsr_pl, +MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE ); + err =mlxsw_reg_write (mlxsw_pci->core,MLXSW_REG (mrsr), mrsr_pl); +if (err) +return err; + +/* Save the PCI configuration space so that we will be able to restore +* it after the firmware was reset. +*/ +pci_save_state (pdev); +pci_cfg_access_lock (pdev); + + err =mlxsw_pci_link_toggle (bridge_pdev); +if (err) { +pci_err (bridge_pdev,"Failed to toggle PCI link\n" ); + goto restore; + } + + err =mlxsw_pci_device_id_read (pdev, dev_id); + +restore: +pci_cfg_access_unlock (pdev); +pci_restore_state (pdev); +return err; +} +static int mlxsw_pci_reset_sw (struct mlxsw_pci *mlxsw_pci) {char mrsr_pl[MLXSW_REG_MRSR_LEN ];@@ -1471,6 +1602,8 @@ static int mlxsw_pci_reset(struct mlxsw_pci *mlxsw_pci, const struct pci_device_id *id) { struct pci_dev *pdev = mlxsw_pci->pdev; +char mcam_pl[MLXSW_REG_MCAM_LEN ]; +bool pci_reset_supported;u32 sys_status;int err;@@ -1481,7 +1614,23 @@ mlxsw_pci_reset(struct mlxsw_pci *mlxsw_pci, const struct pci_device_id *id)return err; } - err =mlxsw_pci_reset_sw (mlxsw_pci); +mlxsw_reg_mcam_pack (mcam_pl, +MLXSW_REG_MCAM_FEATURE_GROUP_ENHANCED_FEATURES ); + err =mlxsw_reg_query (mlxsw_pci->core,MLXSW_REG (mcam), mcam_pl); +if (err) +return err; + +mlxsw_reg_mcam_unpack (mcam_pl,MLXSW_REG_MCAM_PCI_RESET , + &pci_reset_supported); + +if (pci_reset_supported) { +pci_dbg (pdev,"Starting PCI reset flow\n" ); + err =mlxsw_pci_reset_at_pci_disable (mlxsw_pci); + }else { +pci_dbg (pdev,"Starting software reset flow\n" ); + err =mlxsw_pci_reset_sw (mlxsw_pci); + } +if (err)return err;drivers/net/ethernet/mellanox/mlxsw/pci_hw.h +5
-0@@ -27,6 +27,11 @@#define MLXSW_PCI_SW_RESET_TIMEOUT_MSECS 900000#define MLXSW_PCI_SW_RESET_WAIT_MSECS 200 + +#define MLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS 500 +#define MLXSW_PCI_TOGGLE_WAIT_MSECS 20 +#define MLXSW_PCI_TOGGLE_TIMEOUT_MSECS 2000 +#define MLXSW_PCI_FW_READY 0xA1844#define MLXSW_PCI_FW_READY_MASK 0xFFFF#define MLXSW_PCI_FW_READY_MAGIC 0x5ESignature · 1 line
2.39.0
Ido Schimmel
maintainerRe: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow
Hi Bjorn,
Quoted 14 lines · Bjorn Helgaas
Hi Petr, thanks for pointing me here.
Quoted 7 lines · Petr Machata
From: Amit Cohen <[email protected]>
The driver resets the device during probe and during a devlink reload. The current reset method reloads the current firmware version or a pending one, if one was previously flashed using devlink. However, the reset does not take down the PCI link, preventing the PCI firmware from being upgraded, unless the system is rebooted.
Just to make sure I understand this correctly, the above sounds like "firmware" includes two parts that have different rules for loading:
Quoted 6 lines
- Current reset method is completely mlxsw-specific and resets the mlxsw core but not the PCIe interface; this loads only firmware part A
- A PCIe reset resets both the mlxsw core and the PCIe interface; this loads both firmware part A and part B
Yes. A few years ago I had to flash a new firmware in order to test a fix in the PCIe firmware and the bug still reproduced after a devlink reload. Only after a reboot the new PCIe firmware was loaded and the bug was fixed. Bugs in PCIe firmware are not common, but we would like to avoid the scenario where users must reboot the machine in order to load the new firmware.
Quoted 20 lines
Quoted 6 lines
To solve this problem, a new reset command (6) was implemented in the firmware. Unlike the current command (1), after issuing the new command the device will not start the reset immediately, but only after the PCI link was disabled. The driver is expected to wait for 500ms before re-enabling the link to give the firmware enough time to start the reset.
I guess the idea here is that the mlxsw driver:
- Tells the firmware we're going to reset
(MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE)- Saves PCI config state
- Disables the link (
mlxsw_pci_link_toggle()), which causes a PCIe hot reset- The firmware notices the link disable and starts its own internal reset
Quoted 7 lines
- The mlxsw driver waits 500ms
(MLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS)- Enables link and waits for it to be active
(mlxsw_pci_link_active_check()- Waits for device to be ready again (
mlxsw_pci_device_id_read())
Correct.
Quoted 8 lines
So the first question is why you don't simply use
pci_reset_function(), since it is supposed to cause a reset and do all the necessary waiting for the device to be ready. This is quite complicated to do correctly; in fact, we still discover issues there regularly. There are many special cases in PCIe r6.0, sec 6.6.1, and it would be much better if we can avoid trying to handle them all in individual drivers.
I see that this function takes the device lock and I think (didn't try) it will deadlock if we were to replace the current code with it since we also perform a reset during probe where I believe the device lock is already taken.
__pci_reset_function_locked() is another option, but it assumes the device lock was already taken, which is correct during probe, but not when reset is performed as part of devlink reload.
Let's put the locking issues aside and assume we can use __pci_reset_function_locked(). I'm trying to figure out what it can allow us to remove from the driver in favor of common PCI code. It essentially invokes one of the supported reset methods. Looking at my device, I see the following:
# cat /sys/class/pci_bus/0000\:01/device/0000\:01\:00.0/reset_method pm bus
So I assume it will invoke pci_pm_reset(). I'm not sure it can work for us as our reset procedure requires us to disable the link on the downstream port as a way of notifying the device that it should start the reset procedure.
We might be able to use the "device_specific" method and add quirks in "pci_dev_reset_methods". However, I'm not sure what would be the benefit, as it basically means moving the code in mlxsw_pci_link_toggle() to drivers/pci/quirks.c. Also, when the "probe" argument is "true" we can't actually determine if this reset method is supported or not, as we can't query that from the configuration space of the device in the current implementation. It's queried using a command interface that is specific to mlxsw and resides in the driver itself. Not usable from drivers/pci/quirks.c.
Quoted 8 lines
Of course,
pci_reset_function()does *not* include details likeMLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS.I assume that flashing the firmware to the device followed by a power cycle (without ever doing
MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE) would load the new firmware everywhere. Can we not do the same with a PCIe reset?
Yes, that's what we would like to achieve.
Thanks for the feedback!
Re: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow
[+cc Alex, Lukas for link-disable reset thoughts, beginning of thread:
https://lore.kernel.org/all/[email protected]/]
Quoted 79 lines · Ido Schimmel
Quoted 12 lines · Bjorn Helgaas
Quoted 7 lines · Petr Machata
From: Amit Cohen <[email protected]>
The driver resets the device during probe and during a devlink reload. The current reset method reloads the current firmware version or a pending one, if one was previously flashed using devlink. However, the reset does not take down the PCI link, preventing the PCI firmware from being upgraded, unless the system is rebooted.
Just to make sure I understand this correctly, the above sounds like "firmware" includes two parts that have different rules for loading:
Quoted 6 lines
- Current reset method is completely mlxsw-specific and resets the mlxsw core but not the PCIe interface; this loads only firmware part A
- A PCIe reset resets both the mlxsw core and the PCIe interface; this loads both firmware part A and part B
Yes. A few years ago I had to flash a new firmware in order to test a fix in the PCIe firmware and the bug still reproduced after a devlink reload. Only after a reboot the new PCIe firmware was loaded and the bug was fixed. Bugs in PCIe firmware are not common, but we would like to avoid the scenario where users must reboot the machine in order to load the new firmware.
Quoted 19 lines
Quoted 5 lines
To solve this problem, a new reset command (6) was implemented in the firmware. Unlike the current command (1), after issuing the new command the device will not start the reset immediately, but only after the PCI link was disabled. The driver is expected to wait for 500ms before re-enabling the link to give the firmware enough time to start the reset.
I guess the idea here is that the mlxsw driver:
- Tells the firmware we're going to reset
(MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE)- Saves PCI config state
- Disables the link (
mlxsw_pci_link_toggle()), which causes a PCIe hot reset- The firmware notices the link disable and starts its own internal reset
Quoted 7 lines
- The mlxsw driver waits 500ms
(MLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS)- Enables link and waits for it to be active
(mlxsw_pci_link_active_check()- Waits for device to be ready again (
mlxsw_pci_device_id_read())Correct.
Quoted 7 lines
So the first question is why you don't simply use
pci_reset_function(), since it is supposed to cause a reset and do all the necessary waiting for the device to be ready. This is quite complicated to do correctly; in fact, we still discover issues there regularly. There are many special cases in PCIe r6.0, sec 6.6.1, and it would be much better if we can avoid trying to handle them all in individual drivers.I see that this function takes the device lock and I think (didn't try) it will deadlock if we were to replace the current code with it since we also perform a reset during probe where I believe the device lock is already taken.
__pci_reset_function_locked()is another option, but it assumes the device lock was already taken, which is correct during probe, but not when reset is performed as part of devlink reload.Let's put the locking issues aside and assume we can use
__pci_reset_function_locked(). I'm trying to figure out what it can allow us to remove from the driver in favor of common PCI code. It essentially invokes one of the supported reset methods. Looking at my device, I see the following:
Quoted 7 lines
# cat /sys/class/pci_bus/0000\:01/device/0000\:01\:00.0/reset_method pm busSo I assume it will invoke
pci_pm_reset(). I'm not sure it can work for us as our reset procedure requires us to disable the link on the downstream port as a way of notifying the device that it should start the reset procedure.
Hmmm, pci_pm_reset() puts the device in D3hot, then back to D0. Spec says that results in "undefined internal Function state," which doesn't even sound like a guaranteed reset, but it's what we have, and in any case, it does not disable the link.
Quoted 9 lines
We might be able to use the "
device_specific" method and add quirks in "pci_dev_reset_methods". However, I'm not sure what would be the benefit, as it basically means moving the code inmlxsw_pci_link_toggle()todrivers/pci/quirks.c. Also, when the "probe" argument is "true" we can't actually determine if this reset method is supported or not, as we can't query that from the configuration space of the device in the current implementation. It's queried using a command interface that is specific to mlxsw and resides in the driver itself. Not usable fromdrivers/pci/quirks.c.
Spec (PCIe r6.0, sec 6.6.1) says "Disabling a Link causes Downstream components to undergo a hot reset." That seems like it *could* be a general-purpose method of doing a reset, and I don't know why the PCI core doesn't support it. Added Alex and Lukas in case they know.
But it sounds like there's some wrinkle with your device? I suppose a link disable actually causes a reset, but that reset may not trigger the firmware reload you need? If we had a generic "disable link" reset method, maybe a device quirk could disable it if necessary?
Quoted 11 lines
Quoted 7 lines
Of course,
pci_reset_function()does *not* include details likeMLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS.I assume that flashing the firmware to the device followed by a power cycle (without ever doing
MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE) would load the new firmware everywhere. Can we not do the same with a PCIe reset?Yes, that's what we would like to achieve.
Thanks for the feedback!
Re: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow
Quoted 106 lines · On Wed, 29 Mar 2023 11:01:44 -0500 Bjorn Helgaas <[email protected]> wrote:
[+cc Alex, Lukas for link-disable reset thoughts, beginning of thread:
https://lore.kernel.org/all/[email protected]/]Quoted 79 lines · Ido Schimmel
Quoted 12 lines · Bjorn Helgaas
Quoted 7 lines · Petr Machata
From: Amit Cohen <[email protected]>
The driver resets the device during probe and during a devlink reload. The current reset method reloads the current firmware version or a pending one, if one was previously flashed using devlink. However, the reset does not take down the PCI link, preventing the PCI firmware from being upgraded, unless the system is rebooted.
Just to make sure I understand this correctly, the above sounds like "firmware" includes two parts that have different rules for loading:
Quoted 6 lines
- Current reset method is completely mlxsw-specific and resets the mlxsw core but not the PCIe interface; this loads only firmware part A
- A PCIe reset resets both the mlxsw core and the PCIe interface; this loads both firmware part A and part B
Yes. A few years ago I had to flash a new firmware in order to test a fix in the PCIe firmware and the bug still reproduced after a devlink reload. Only after a reboot the new PCIe firmware was loaded and the bug was fixed. Bugs in PCIe firmware are not common, but we would like to avoid the scenario where users must reboot the machine in order to load the new firmware.
Quoted 19 lines
Quoted 5 lines
To solve this problem, a new reset command (6) was implemented in the firmware. Unlike the current command (1), after issuing the new command the device will not start the reset immediately, but only after the PCI link was disabled. The driver is expected to wait for 500ms before re-enabling the link to give the firmware enough time to start the reset.
I guess the idea here is that the mlxsw driver:
- Tells the firmware we're going to reset
(MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE)- Saves PCI config state
- Disables the link (
mlxsw_pci_link_toggle()), which causes a PCIe hot reset- The firmware notices the link disable and starts its own internal reset
Quoted 7 lines
- The mlxsw driver waits 500ms
(MLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS)- Enables link and waits for it to be active
(mlxsw_pci_link_active_check()- Waits for device to be ready again (
mlxsw_pci_device_id_read())Correct.
Quoted 7 lines
So the first question is why you don't simply use
pci_reset_function(), since it is supposed to cause a reset and do all the necessary waiting for the device to be ready. This is quite complicated to do correctly; in fact, we still discover issues there regularly. There are many special cases in PCIe r6.0, sec 6.6.1, and it would be much better if we can avoid trying to handle them all in individual drivers.I see that this function takes the device lock and I think (didn't try) it will deadlock if we were to replace the current code with it since we also perform a reset during probe where I believe the device lock is already taken.
__pci_reset_function_locked()is another option, but it assumes the device lock was already taken, which is correct during probe, but not when reset is performed as part of devlink reload.Let's put the locking issues aside and assume we can use
__pci_reset_function_locked(). I'm trying to figure out what it can allow us to remove from the driver in favor of common PCI code. It essentially invokes one of the supported reset methods. Looking at my device, I see the following:Quoted 7 lines
# cat /sys/class/pci_bus/0000\:01/device/0000\:01\:00.0/reset_method pm busSo I assume it will invoke
pci_pm_reset(). I'm not sure it can work for us as our reset procedure requires us to disable the link on the downstream port as a way of notifying the device that it should start the reset procedure.Hmmm,
pci_pm_reset()puts the device in D3hot, then back to D0. Spec says that results in "undefined internal Function state," which doesn't even sound like a guaranteed reset, but it's what we have, and in any case, it does not disable the link.Quoted 9 lines
We might be able to use the "
device_specific" method and add quirks in "pci_dev_reset_methods". However, I'm not sure what would be the benefit, as it basically means moving the code inmlxsw_pci_link_toggle()todrivers/pci/quirks.c. Also, when the "probe" argument is "true" we can't actually determine if this reset method is supported or not, as we can't query that from the configuration space of the device in the current implementation. It's queried using a command interface that is specific to mlxsw and resides in the driver itself. Not usable fromdrivers/pci/quirks.c.
Quoted 4 lines
Spec (PCIe r6.0, sec 6.6.1) says "Disabling a Link causes Downstream components to undergo a hot reset." That seems like it *could* be a general-purpose method of doing a reset, and I don't know why the PCI core doesn't support it. Added Alex and Lukas in case they know.
I think we don't have it because it's unclear how it's actually different from a secondary bus reset from the bridge control register, which is what "bus" would do when selected for the example above. Per the spec, both must cause a hot reset. It seems this device needs a significantly longer delay though.
Note that hot resets can be generated by a userspace driver with ownership of the device and will make use of the pci-core reset mechanisms. Therefore if there is not a device specific reset, we'll use the standard delays and the device ought not to get itself wedged if the link becomes active at an unexpected point relative to a firmware update. This might be a point in favor of a device specific reset solution in pci-core. Thanks,
Alex
Quoted 17 lines
But it sounds like there's some wrinkle with your device? I suppose a link disable actually causes a reset, but that reset may not trigger the firmware reload you need? If we had a generic "disable link" reset method, maybe a device quirk could disable it if necessary?
Quoted 11 lines
Quoted 7 lines
Of course,
pci_reset_function()does *not* include details likeMLXSW_PCI_TOGGLE_WAIT_BEFORE_EN_MSECS.I assume that flashing the firmware to the device followed by a power cycle (without ever doing
MLXSW_REG_MRSR_COMMAND_RESET_AT_PCI_DISABLE) would load the new firmware everywhere. Can we not do the same with a PCIe reset?Yes, that's what we would like to achieve.
Thanks for the feedback!
Ido Schimmel
maintainerRe: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow
Quoted 5 lines · Alex Williamson
I think we don't have it because it's unclear how it's actually different from a secondary bus reset from the bridge control register, which is what "bus" would do when selected for the example above. Per the spec, both must cause a hot reset. It seems this device needs a significantly longer delay though.
Assuming you are referring to the 2ms sleep in pci_reset_secondary_bus(), then yes. In our case, after disabling the link on the downstream port we need to wait for 500ms before enabling it.
Quoted 7 lines
Note that hot resets can be generated by a userspace driver with ownership of the device and will make use of the pci-core reset mechanisms. Therefore if there is not a device specific reset, we'll use the standard delays and the device ought not to get itself wedged if the link becomes active at an unexpected point relative to a firmware update. This might be a point in favor of a device specific reset solution in pci-core. Thanks,
I assume you referring to something like this:
# echo 1 > /sys/class/pci_bus/0000:03/device/0000:03:00.0/reset
Doesn't seem to have any effect (network ports remain up, at least). Anyway, this device is completely managed by the kernel, not a user space driver. I'm not aware of anyone using this method to reset the device.
If I understand Bjorn and you correctly, we have two options:
1. Keep the current implementation inside the driver.
2. Call __pci_reset_function_locked() from the driver and move the link toggling to drivers/pci/quirks.c as a "device_specific" method.
Personally, I don't see any benefit in 2, but we can try to implement it, see if it even works and then decide.
Please let me know what are your preferences.
Thanks
Re: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow
Quoted 20 lines · On Thu, 30 Mar 2023 11:26:30 +0300 Ido Schimmel <[email protected]> wrote:
Quoted 5 lines · Alex Williamson
I think we don't have it because it's unclear how it's actually different from a secondary bus reset from the bridge control register, which is what "bus" would do when selected for the example above. Per the spec, both must cause a hot reset. It seems this device needs a significantly longer delay though.
Assuming you are referring to the 2ms sleep in
pci_reset_secondary_bus(), then yes. In our case, after disabling the link on the downstream port we need to wait for 500ms before enabling it.Quoted 7 lines
Note that hot resets can be generated by a userspace driver with ownership of the device and will make use of the pci-core reset mechanisms. Therefore if there is not a device specific reset, we'll use the standard delays and the device ought not to get itself wedged if the link becomes active at an unexpected point relative to a firmware update. This might be a point in favor of a device specific reset solution in pci-core. Thanks,
Quoted 8 lines
I assume you referring to something like this:
# echo 1 > /sys/class/pci_bus/0000:03/device/0000:03:00.0/resetDoesn't seem to have any effect (network ports remain up, at least). Anyway, this device is completely managed by the kernel, not a user space driver. I'm not aware of anyone using this method to reset the device.
The pci-sysfs reset attribute is only meant to reset the linked device, so if this is a single function device then it might be accessing bus reset, but it also might be using FLR or PM reset. There's a reset_method attribute to determine and select.
In any case, if the device is unaffected, that suggests we're dealing with a device that doesn't comply with PCIe reset standards, which might suggests it needs a device specific reset or to flag broken reset methods regardless.
Note that QEMU is a vfio-pci userspace driver, so assigning the device to a VM, where kernel drivers in the guest are managing the device is a use case of userspace drivers which should have a functional reset mechanism to avoid data leakage between userspace sessions.
Quoted 9 lines
If I understand Bjorn and you correctly, we have two options:
1. Keep the current implementation inside the driver.
2. Call
__pci_reset_function_locked()from the driver and move the link toggling todrivers/pci/quirks.cas a "device_specific" method.Personally, I don't see any benefit in 2, but we can try to implement it, see if it even works and then decide.
The second option enables use cases like above, where the PCI-core can perform an effective reset of the device rather than embedding that into a specific driver. Even if not intended as a primary use case, it's a more complete solution and avoids potentially unhappy users that assume such use cases are available. Thanks,
Alex
Ido Schimmel
maintainerRe: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow
Quoted 54 lines · Alex Williamson
Quoted 20 lines · On Thu, 30 Mar 2023 11:26:30 +0300 Ido Schimmel <[email protected]> wrote:
Quoted 5 lines · Alex Williamson
I think we don't have it because it's unclear how it's actually different from a secondary bus reset from the bridge control register, which is what "bus" would do when selected for the example above. Per the spec, both must cause a hot reset. It seems this device needs a significantly longer delay though.
Assuming you are referring to the 2ms sleep in
pci_reset_secondary_bus(), then yes. In our case, after disabling the link on the downstream port we need to wait for 500ms before enabling it.Quoted 7 lines
Note that hot resets can be generated by a userspace driver with ownership of the device and will make use of the pci-core reset mechanisms. Therefore if there is not a device specific reset, we'll use the standard delays and the device ought not to get itself wedged if the link becomes active at an unexpected point relative to a firmware update. This might be a point in favor of a device specific reset solution in pci-core. Thanks,
Quoted 8 lines
I assume you referring to something like this:
# echo 1 > /sys/class/pci_bus/0000:03/device/0000:03:00.0/resetDoesn't seem to have any effect (network ports remain up, at least). Anyway, this device is completely managed by the kernel, not a user space driver. I'm not aware of anyone using this method to reset the device.
The pci-sysfs reset attribute is only meant to reset the linked device, so if this is a single function device then it might be accessing bus reset, but it also might be using FLR or PM reset. There's a
reset_methodattribute to determine and select.In any case, if the device is unaffected, that suggests we're dealing with a device that doesn't comply with PCIe reset standards, which might suggests it needs a device specific reset or to flag broken reset methods regardless.
Note that QEMU is a vfio-pci userspace driver, so assigning the device to a VM, where kernel drivers in the guest are managing the device is a use case of userspace drivers which should have a functional reset mechanism to avoid data leakage between userspace sessions.
Quoted 7 lines
If I understand Bjorn and you correctly, we have two options:
1. Keep the current implementation inside the driver.
2. Call
__pci_reset_function_locked()from the driver and move the link toggling todrivers/pci/quirks.cas a "device_specific" method.
Quoted 8 lines
Quoted 2 lines
Personally, I don't see any benefit in 2, but we can try to implement it, see if it even works and then decide.
The second option enables use cases like above, where the PCI-core can perform an effective reset of the device rather than embedding that into a specific driver. Even if not intended as a primary use case, it's a more complete solution and avoids potentially unhappy users that assume such use cases are available. Thanks,
I believe we are discussing three different issues:
1. Reset by PCI core when driver is bound to the device.
2. Reset by PCI core when driver is not bound to the device.
3. Reset by the driver itself during probe / devlink reload.
These are my notes on each:
1. In this case, the reset_prepare() and reset_done() handlers of the driver are invoked by the PCI core before and after the PCI reset. Without them, if the used PCI reset method indeed reset the entire device, then the driver and the device would be out of sync. I have implemented these handlers for the driver:
https://github.com/idosch/linux/commit/28bc0dc06c01559c19331578bbba9f2b0460ab5d.patch
2. In this case, the handlers are not available and we only need to ensure that the used PCI reset method reset the device. The method can be a generic method such as "pm" or "bus" (which are available in my case) or a "device_specific" method that is implemented in drivers/pci/quirks.c by accessing the configuration space of the device.
I need to check if one of the generic methods works for this device and if not, check with the PCI team what we can do about it.
3. In this case, the driver already issues a reset via its command interface during probe / devlink reload to ensure it is working with a device in a clean state. The patch we are discussing merely adds another reset method. Instead of starting the reset when the command is issued, the reset will start after toggling the link on the downstream port.
To summarize, I would like to proceed as follows:
1. Submit the following patch that implements the PCI reset handlers for the device:
https://github.com/idosch/linux/commit/28bc0dc06c01559c19331578bbba9f2b0460ab5d.patch
2. Check if one of the generic reset methods works for this device and if not, check with the PCI team what we can do about it.
3. Re-submit the current patchset for inclusion.
Note that 2 does not block 3 since the solution for 2 would be either in the firmware or in drivers/pci/quirks.c.
Please let me know if you are OK with the above.
Thanks
Jakub Kicinski
maintainerRe: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow
Quoted 30 lines · On Thu, 13 Apr 2023 11:10:54 +0300 Ido Schimmel wrote:
I believe we are discussing three different issues:
1. Reset by PCI core when driver is bound to the device.
2. Reset by PCI core when driver is not bound to the device.
3. Reset by the driver itself during probe / devlink reload.These are my notes on each:
1. In this case, the
reset_prepare()andreset_done()handlers of the driver are invoked by the PCI core before and after the PCI reset. Without them, if the used PCI reset method indeed reset the entire device, then the driver and the device would be out of sync. I have implemented these handlers for the driver:
https://github.com/idosch/linux/commit/28bc0dc06c01559c19331578bbba9f2b0460ab5d.patch2. In this case, the handlers are not available and we only need to ensure that the used PCI reset method reset the device. The method can be a generic method such as "pm" or "bus" (which are available in my case) or a "
device_specific" method that is implemented indrivers/pci/quirks.cby accessing the configuration space of the device.I need to check if one of the generic methods works for this device and if not, check with the PCI team what we can do about it.
3. In this case, the driver already issues a reset via its command interface during probe / devlink reload to ensure it is working with a device in a clean state. The patch we are discussing merely adds another reset method. Instead of starting the reset when the command is issued, the reset will start after toggling the link on the downstream port.
Quoted 5 lines
To summarize, I would like to proceed as follows:
1. Submit the following patch that implements the PCI reset handlers for the device:
https://github.com/idosch/linux/commit/28bc0dc06c01559c19331578bbba9f2b0460ab5d.patch
I'm probably confused but does this mean that PCI reset would impact the datapath? From the commit message it sounded like the new reset is harder than the previous one.
Re: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow
In reply to Bjorn HelgaasQuoted 5 lines · Bjorn Helgaas
Quoted 4 lines · Ido Schimmel
Quoted 3 lines · Bjorn Helgaas
So the first question is why you don't simply use
pci_reset_function(), since it is supposed to cause a reset and do all the necessary waiting for the device to be ready.
I agree, this driver should use one of the reset helpers provided by the PCI core.
Not least because if the Downstream Port is hotplug-capable, fiddling with the Link Disable bit behind the hotplug driver's back will cause unbinding and rebinding of the device.
Quoted 3 lines
Quoted 3 lines
__pci_reset_function_locked()is another option, but it assumes the device lock was already taken, which is correct during probe, but not when reset is performed as part of devlink reload.
Just call pci_dev_lock() in the devlink reload code path.
Quoted 4 lines
Hmmm,
pci_pm_reset()puts the device in D3hot, then back to D0. Spec says that results in "undefined internal Function state," which doesn't even sound like a guaranteed reset, but it's what we have, and in any case, it does not disable the link.
Per PCIe r6.0.1 sec 5.8, the device undergoes a Soft Reset when moving from D3hot to D0 (unless the No_Soft_Reset bit is set in the Power Management Control/Status Register, sec 7.5.2.2).
The driver can set the PCI_DEV_FLAGS_NO_PM_RESET flag if this reset method doesn't have any effect (via quirk_no_pm_reset()).
We could also discuss moving pci_pm_reset after pci_reset_bus_function in pci_reset_fn_methods[] if this reset method has little value on the majority of devices.
Alternatively, if Secondary Bus Reset has the intended effect, the driver could invoke that directly via pci_reset_bus().
Quoted 4 lines
Spec (PCIe r6.0, sec 6.6.1) says "Disabling a Link causes Downstream components to undergo a hot reset." That seems like it *could* be a general-purpose method of doing a reset, and I don't know why the PCI core doesn't support it. Added Alex and Lukas in case they know.
Sounds reasonable to me.
Thanks,
Lukas