* [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830 @ 2025-02-19 0:46 Jacob Keller 2025-02-19 9:37 ` Michal Swiatkowski 0 siblings, 1 reply; 5+ messages in thread From: Jacob Keller @ 2025-02-19 0:46 UTC (permalink / raw) To: Tony Nguyen, Przemek Kitszel Cc: intel-wired-lan, netdev, Paul Greenwalt, Jacob Keller From: Paul Greenwalt <paul.greenwalt@intel.com> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when setting the AQ command read flag, and since the get command is a direct command there is no need to set the read flag. Fix this by only setting read flag on set command. Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644 --- a/drivers/net/ethernet/intel/ice/ice_ddp.c +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c @@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size, cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM | ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW; - if (ice_is_e825c(hw)) - desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); + desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); } else { ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo); cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM; - } - if (!ice_is_e825c(hw)) - desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); + if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 && + hw->mac_type != ICE_MAC_E830) + desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); + } status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd); if (status) --- base-commit: f5da7c45188eea71394bf445655cae2df88a7788 change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a Best regards, -- Jacob Keller <jacob.e.keller@intel.com> ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830 2025-02-19 0:46 [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830 Jacob Keller @ 2025-02-19 9:37 ` Michal Swiatkowski 2025-02-20 22:45 ` Jacob Keller 0 siblings, 1 reply; 5+ messages in thread From: Michal Swiatkowski @ 2025-02-19 9:37 UTC (permalink / raw) To: Jacob Keller Cc: Tony Nguyen, Przemek Kitszel, intel-wired-lan, netdev, Paul Greenwalt On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote: > From: Paul Greenwalt <paul.greenwalt@intel.com> > > With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when > setting the AQ command read flag, and since the get command is a direct > command there is no need to set the read flag. > > Fix this by only setting read flag on set command. Why it isn't true for other hw? I mean, why not: if (set) RD_FLAG else NOT_RD_FLAG Other hw needs RD flag in case of get too? > Don't you need fixes tag? > Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c > index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ddp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c > @@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size, > cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM | > ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW; > > - if (ice_is_e825c(hw)) > - desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); > + desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); > } else { > ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo); > cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM; > - } > > - if (!ice_is_e825c(hw)) > - desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); > + if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 && > + hw->mac_type != ICE_MAC_E830) > + desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); > + } > > status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd); > if (status) > In general looks fine, only one question. Thanks, Michal > --- > base-commit: f5da7c45188eea71394bf445655cae2df88a7788 > change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a > > Best regards, > -- > Jacob Keller <jacob.e.keller@intel.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830 2025-02-19 9:37 ` Michal Swiatkowski @ 2025-02-20 22:45 ` Jacob Keller 2025-02-21 7:29 ` Michal Swiatkowski 0 siblings, 1 reply; 5+ messages in thread From: Jacob Keller @ 2025-02-20 22:45 UTC (permalink / raw) To: Michal Swiatkowski Cc: Tony Nguyen, Przemek Kitszel, intel-wired-lan, netdev, Paul Greenwalt On 2/19/2025 1:37 AM, Michal Swiatkowski wrote: > On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote: >> From: Paul Greenwalt <paul.greenwalt@intel.com> >> >> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when >> setting the AQ command read flag, and since the get command is a direct >> command there is no need to set the read flag. >> >> Fix this by only setting read flag on set command. > > Why it isn't true for other hw? I mean, why not: > if (set) > RD_FLAG > else > NOT_RD_FLAG > Other hw needs RD flag in case of get too? > From what I understand, we didn't anticipate this flow changing. E810 and E822 hardware require FLAG_RD for both get and set, while E825-C and E830 expect FLAG_RD only for set, but not for get. >> > > Don't you need fixes tag? You're correct. I'll add it in v2 >> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >> --- >> drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c >> index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c >> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c >> @@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size, >> cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM | >> ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW; >> >> - if (ice_is_e825c(hw)) >> - desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); >> + desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); >> } else { >> ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo); >> cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM; >> - } >> >> - if (!ice_is_e825c(hw)) >> - desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); >> + if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 && >> + hw->mac_type != ICE_MAC_E830) >> + desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); >> + } >> >> status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd); >> if (status) >> > > In general looks fine, only one question. > > Thanks, > Michal Thanks for the review, I'll send a v2 with this cleaned up and include a fixes tag. > >> --- >> base-commit: f5da7c45188eea71394bf445655cae2df88a7788 >> change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a >> >> Best regards, >> -- >> Jacob Keller <jacob.e.keller@intel.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830 2025-02-20 22:45 ` Jacob Keller @ 2025-02-21 7:29 ` Michal Swiatkowski 2025-02-21 22:41 ` Jacob Keller 0 siblings, 1 reply; 5+ messages in thread From: Michal Swiatkowski @ 2025-02-21 7:29 UTC (permalink / raw) To: Jacob Keller Cc: Michal Swiatkowski, Tony Nguyen, Przemek Kitszel, intel-wired-lan, netdev, Paul Greenwalt On Thu, Feb 20, 2025 at 02:45:41PM -0800, Jacob Keller wrote: > > > On 2/19/2025 1:37 AM, Michal Swiatkowski wrote: > > On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote: > >> From: Paul Greenwalt <paul.greenwalt@intel.com> > >> > >> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when > >> setting the AQ command read flag, and since the get command is a direct > >> command there is no need to set the read flag. > >> > >> Fix this by only setting read flag on set command. > > > > Why it isn't true for other hw? I mean, why not: > > if (set) > > RD_FLAG > > else > > NOT_RD_FLAG > > Other hw needs RD flag in case of get too? > > > > From what I understand, we didn't anticipate this flow changing. E810 > and E822 hardware require FLAG_RD for both get and set, while E825-C and > E830 expect FLAG_RD only for set, but not for get. > Thanks for explanation. Seems resonable from driver perspective and not so reasonable from firmware, but maybe this difference is somehow important. Thanks, Michal > >> > > > > Don't you need fixes tag? > > You're correct. I'll add it in v2 > > >> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> > >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > >> --- > >> drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++----- > >> 1 file changed, 5 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c > >> index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644 > >> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c > >> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c > >> @@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size, > >> cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM | > >> ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW; > >> > >> - if (ice_is_e825c(hw)) > >> - desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); > >> + desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); > >> } else { > >> ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo); > >> cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM; > >> - } > >> > >> - if (!ice_is_e825c(hw)) > >> - desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); > >> + if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 && > >> + hw->mac_type != ICE_MAC_E830) > >> + desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD); > >> + } > >> > >> status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd); > >> if (status) > >> > > > > In general looks fine, only one question. > > > > Thanks, > > Michal > > Thanks for the review, I'll send a v2 with this cleaned up and include a > fixes tag. > > > > >> --- > >> base-commit: f5da7c45188eea71394bf445655cae2df88a7788 > >> change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a > >> > >> Best regards, > >> -- > >> Jacob Keller <jacob.e.keller@intel.com> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830 2025-02-21 7:29 ` Michal Swiatkowski @ 2025-02-21 22:41 ` Jacob Keller 0 siblings, 0 replies; 5+ messages in thread From: Jacob Keller @ 2025-02-21 22:41 UTC (permalink / raw) To: Michal Swiatkowski Cc: Tony Nguyen, Przemek Kitszel, intel-wired-lan, netdev, Paul Greenwalt On 2/20/2025 11:29 PM, Michal Swiatkowski wrote: > On Thu, Feb 20, 2025 at 02:45:41PM -0800, Jacob Keller wrote: >> >> >> On 2/19/2025 1:37 AM, Michal Swiatkowski wrote: >>> On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote: >>>> From: Paul Greenwalt <paul.greenwalt@intel.com> >>>> >>>> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when >>>> setting the AQ command read flag, and since the get command is a direct >>>> command there is no need to set the read flag. >>>> >>>> Fix this by only setting read flag on set command. >>> >>> Why it isn't true for other hw? I mean, why not: >>> if (set) >>> RD_FLAG >>> else >>> NOT_RD_FLAG >>> Other hw needs RD flag in case of get too? >>> >> >> From what I understand, we didn't anticipate this flow changing. E810 >> and E822 hardware require FLAG_RD for both get and set, while E825-C and >> E830 expect FLAG_RD only for set, but not for get. >> > > Thanks for explanation. Seems resonable from driver perspective and not > so reasonable from firmware, but maybe this difference is somehow > important. > > Thanks, > Michal > Yea, its unfortunate that this changed at all, but we can't really go back on released firmware versions. The RD_FLAG is described in the datasheet as: Flags.RD: Set by driver to indicate that FW needs to read indirect buffer. It is intended to indicate the presence of the indirect buffer vs a direct command. I believe what has likely happened is that this is interpreted in a strict manner so missing the flag causes the commands to fail. Its quite weird that older firmware required it for a direct command, so someone probably changed this thinking thats strange.. I haven't been able to dig up more info about this or why it was changed. Thanks, Jake ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-02-21 22:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-02-19 0:46 [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830 Jacob Keller 2025-02-19 9:37 ` Michal Swiatkowski 2025-02-20 22:45 ` Jacob Keller 2025-02-21 7:29 ` Michal Swiatkowski 2025-02-21 22:41 ` Jacob Keller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).