| Message ID | 20230801115235.67343-1-marcin.szycik@linux.intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net] ice: Block switchdev mode when ADQ is acvite and vice versa | expand |
On 8/1/23 13:52, Marcin Szycik wrote: > ADQ and switchdev are not supported simultaneously. Enabling both at the > same time can result in nullptr dereference. > > To prevent this, check if ADQ is active when changing devlink mode to > switchdev mode, and check if switchdev is active when enabling ADQ. > > Fixes: fbc7b27af0f9 ("ice: enable ndo_setup_tc support for mqprio_qdisc") > Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> > --- > drivers/net/ethernet/intel/ice/ice_eswitch.c | 5 +++++ > drivers/net/ethernet/intel/ice/ice_main.c | 6 ++++++ > 2 files changed, 11 insertions(+) > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
On Tue, Aug 01, 2023 at 01:52:35PM +0200, Marcin Szycik wrote: > ADQ and switchdev are not supported simultaneously. Enabling both at the > same time can result in nullptr dereference. > > To prevent this, check if ADQ is active when changing devlink mode to > switchdev mode, and check if switchdev is active when enabling ADQ. > > Fixes: fbc7b27af0f9 ("ice: enable ndo_setup_tc support for mqprio_qdisc") > Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> > --- > drivers/net/ethernet/intel/ice/ice_eswitch.c | 5 +++++ > drivers/net/ethernet/intel/ice/ice_main.c | 6 ++++++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c > index ad0a007b7398..2ea5aaceee11 100644 > --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c > @@ -538,6 +538,11 @@ ice_eswitch_mode_set(struct devlink *devlink, u16 mode, > break; > case DEVLINK_ESWITCH_MODE_SWITCHDEV: > { > + if (ice_is_adq_active(pf)) { > + dev_err(ice_pf_to_dev(pf), "switchdev cannot be configured - ADQ is active. Delete ADQ configs using TC and try again\n"); It needs to be reported through netlink extack. > + return -EOPNOTSUPP; > + } > + > dev_info(ice_pf_to_dev(pf), "PF %d changed eswitch mode to switchdev", > pf->hw.pf_id); > NL_SET_ERR_MSG_MOD(extack, "Changed eswitch mode to switchdev"); Thanks
Thu, Aug 03, 2023 at 03:11:26PM CEST, leon@kernel.org wrote: >On Tue, Aug 01, 2023 at 01:52:35PM +0200, Marcin Szycik wrote: >> ADQ and switchdev are not supported simultaneously. Enabling both at the >> same time can result in nullptr dereference. >> >> To prevent this, check if ADQ is active when changing devlink mode to >> switchdev mode, and check if switchdev is active when enabling ADQ. >> >> Fixes: fbc7b27af0f9 ("ice: enable ndo_setup_tc support for mqprio_qdisc") >> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> >> --- >> drivers/net/ethernet/intel/ice/ice_eswitch.c | 5 +++++ >> drivers/net/ethernet/intel/ice/ice_main.c | 6 ++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c >> index ad0a007b7398..2ea5aaceee11 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c >> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c >> @@ -538,6 +538,11 @@ ice_eswitch_mode_set(struct devlink *devlink, u16 mode, >> break; >> case DEVLINK_ESWITCH_MODE_SWITCHDEV: >> { >> + if (ice_is_adq_active(pf)) { >> + dev_err(ice_pf_to_dev(pf), "switchdev cannot be configured - ADQ is active. Delete ADQ configs using TC and try again\n"); Does this provide sufficient hint to the user? I mean, what's ADQ and how it is related to TC objects? Please be more precise. > >It needs to be reported through netlink extack. > >> + return -EOPNOTSUPP; >> + } >> + >> dev_info(ice_pf_to_dev(pf), "PF %d changed eswitch mode to switchdev", >> pf->hw.pf_id); >> NL_SET_ERR_MSG_MOD(extack, "Changed eswitch mode to switchdev"); > >Thanks >
On 03.08.2023 15:11, Leon Romanovsky wrote: > On Tue, Aug 01, 2023 at 01:52:35PM +0200, Marcin Szycik wrote: >> ADQ and switchdev are not supported simultaneously. Enabling both at the >> same time can result in nullptr dereference. >> >> To prevent this, check if ADQ is active when changing devlink mode to >> switchdev mode, and check if switchdev is active when enabling ADQ. >> >> Fixes: fbc7b27af0f9 ("ice: enable ndo_setup_tc support for mqprio_qdisc") >> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> >> --- >> drivers/net/ethernet/intel/ice/ice_eswitch.c | 5 +++++ >> drivers/net/ethernet/intel/ice/ice_main.c | 6 ++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c >> index ad0a007b7398..2ea5aaceee11 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c >> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c >> @@ -538,6 +538,11 @@ ice_eswitch_mode_set(struct devlink *devlink, u16 mode, >> break; >> case DEVLINK_ESWITCH_MODE_SWITCHDEV: >> { >> + if (ice_is_adq_active(pf)) { >> + dev_err(ice_pf_to_dev(pf), "switchdev cannot be configured - ADQ is active. Delete ADQ configs using TC and try again\n"); > > It needs to be reported through netlink extack. Will do, thanks! > >> + return -EOPNOTSUPP; >> + } >> + >> dev_info(ice_pf_to_dev(pf), "PF %d changed eswitch mode to switchdev", >> pf->hw.pf_id); >> NL_SET_ERR_MSG_MOD(extack, "Changed eswitch mode to switchdev"); > > Thanks
On 03.08.2023 15:26, Jiri Pirko wrote: > Thu, Aug 03, 2023 at 03:11:26PM CEST, leon@kernel.org wrote: >> On Tue, Aug 01, 2023 at 01:52:35PM +0200, Marcin Szycik wrote: >>> ADQ and switchdev are not supported simultaneously. Enabling both at the >>> same time can result in nullptr dereference. >>> >>> To prevent this, check if ADQ is active when changing devlink mode to >>> switchdev mode, and check if switchdev is active when enabling ADQ. >>> >>> Fixes: fbc7b27af0f9 ("ice: enable ndo_setup_tc support for mqprio_qdisc") >>> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> >>> --- >>> drivers/net/ethernet/intel/ice/ice_eswitch.c | 5 +++++ >>> drivers/net/ethernet/intel/ice/ice_main.c | 6 ++++++ >>> 2 files changed, 11 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c >>> index ad0a007b7398..2ea5aaceee11 100644 >>> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c >>> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c >>> @@ -538,6 +538,11 @@ ice_eswitch_mode_set(struct devlink *devlink, u16 mode, >>> break; >>> case DEVLINK_ESWITCH_MODE_SWITCHDEV: >>> { >>> + if (ice_is_adq_active(pf)) { >>> + dev_err(ice_pf_to_dev(pf), "switchdev cannot be configured - ADQ is active. Delete ADQ configs using TC and try again\n"); > > Does this provide sufficient hint to the user? I mean, what's ADQ and > how it is related to TC objects? Please be more precise. Application Device Queues, a conflicting feature unrelated to switchdev. If it's enabled, there's a good chance the user knows what it is because they configured it. Could you suggest a better error message? > > >> >> It needs to be reported through netlink extack. >> >>> + return -EOPNOTSUPP; >>> + } >>> + >>> dev_info(ice_pf_to_dev(pf), "PF %d changed eswitch mode to switchdev", >>> pf->hw.pf_id); >>> NL_SET_ERR_MSG_MOD(extack, "Changed eswitch mode to switchdev"); >> >> Thanks >> > Regards, Marcin
Thu, Aug 03, 2023 at 05:11:16PM CEST, marcin.szycik@linux.intel.com wrote: > > >On 03.08.2023 15:26, Jiri Pirko wrote: >> Thu, Aug 03, 2023 at 03:11:26PM CEST, leon@kernel.org wrote: >>> On Tue, Aug 01, 2023 at 01:52:35PM +0200, Marcin Szycik wrote: >>>> ADQ and switchdev are not supported simultaneously. Enabling both at the >>>> same time can result in nullptr dereference. >>>> >>>> To prevent this, check if ADQ is active when changing devlink mode to >>>> switchdev mode, and check if switchdev is active when enabling ADQ. >>>> >>>> Fixes: fbc7b27af0f9 ("ice: enable ndo_setup_tc support for mqprio_qdisc") >>>> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> >>>> --- >>>> drivers/net/ethernet/intel/ice/ice_eswitch.c | 5 +++++ >>>> drivers/net/ethernet/intel/ice/ice_main.c | 6 ++++++ >>>> 2 files changed, 11 insertions(+) >>>> >>>> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c >>>> index ad0a007b7398..2ea5aaceee11 100644 >>>> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c >>>> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c >>>> @@ -538,6 +538,11 @@ ice_eswitch_mode_set(struct devlink *devlink, u16 mode, >>>> break; >>>> case DEVLINK_ESWITCH_MODE_SWITCHDEV: >>>> { >>>> + if (ice_is_adq_active(pf)) { >>>> + dev_err(ice_pf_to_dev(pf), "switchdev cannot be configured - ADQ is active. Delete ADQ configs using TC and try again\n"); >> >> Does this provide sufficient hint to the user? I mean, what's ADQ and >> how it is related to TC objects? Please be more precise. > >Application Device Queues, a conflicting feature unrelated to switchdev. >If it's enabled, there's a good chance the user knows what it is because >they configured it. > >Could you suggest a better error message? The user would need to know what he needs to do in order to make this work. So it would be nice to hint what rules need to be removed. > >> >> >>> >>> It needs to be reported through netlink extack. >>> >>>> + return -EOPNOTSUPP; >>>> + } >>>> + >>>> dev_info(ice_pf_to_dev(pf), "PF %d changed eswitch mode to switchdev", >>>> pf->hw.pf_id); >>>> NL_SET_ERR_MSG_MOD(extack, "Changed eswitch mode to switchdev"); >>> >>> Thanks >>> >> > >Regards, >Marcin
diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c index ad0a007b7398..2ea5aaceee11 100644 --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c @@ -538,6 +538,11 @@ ice_eswitch_mode_set(struct devlink *devlink, u16 mode, break; case DEVLINK_ESWITCH_MODE_SWITCHDEV: { + if (ice_is_adq_active(pf)) { + dev_err(ice_pf_to_dev(pf), "switchdev cannot be configured - ADQ is active. Delete ADQ configs using TC and try again\n"); + return -EOPNOTSUPP; + } + dev_info(ice_pf_to_dev(pf), "PF %d changed eswitch mode to switchdev", pf->hw.pf_id); NL_SET_ERR_MSG_MOD(extack, "Changed eswitch mode to switchdev"); diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index cf92c39467c8..2468b6018613 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -8834,6 +8834,12 @@ ice_setup_tc(struct net_device *netdev, enum tc_setup_type type, } } + if (ice_is_eswitch_mode_switchdev(pf)) { + netdev_err(netdev, "TC MQPRIO offload not supported, switchdev is enabled\n"); + err = -EOPNOTSUPP; + goto adev_unlock; + } + /* setup traffic classifier for receive side */ mutex_lock(&pf->tc_mutex); err = ice_setup_tc_mqprio_qdisc(netdev, type_data);
ADQ and switchdev are not supported simultaneously. Enabling both at the same time can result in nullptr dereference. To prevent this, check if ADQ is active when changing devlink mode to switchdev mode, and check if switchdev is active when enabling ADQ. Fixes: fbc7b27af0f9 ("ice: enable ndo_setup_tc support for mqprio_qdisc") Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> --- drivers/net/ethernet/intel/ice/ice_eswitch.c | 5 +++++ drivers/net/ethernet/intel/ice/ice_main.c | 6 ++++++ 2 files changed, 11 insertions(+)