Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1411096

Re: [PATCH] qed: fix qed_fill_link() error handling

Show all headers | View raw

On Wednesday, June 1, 2016 10:55:02 AM CEST Yuval Mintz wrote:
> > > I think both solutions are equally valid/elegant.
> > >
> > > Arnd?
> > 
> > I think we can just remove the IS_ENABLED() check there and define the
> > IS_PF() macro conditionally to become 'true' if CONFIG_QED_SRIOV is not set,
> > like some other drivers do
> > 
> > diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c
> > b/drivers/net/ethernet/qlogic/qed/qed_main.c
> > index 287f61c20c19..756176525cf9 100644
> > --- a/drivers/net/ethernet/qlogic/qed/qed_main.c
> > +++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
> > @@ -1110,7 +1110,7 @@ static int qed_get_link_data(struct qed_hwfn *hwfn,
> > {
> >       void *p;
> > 
> > -     if (IS_ENABLED(CONFIG_QED_SRIOV) && !IS_PF(hwfn->cdev)) {
> > +     if (!IS_PF(hwfn->cdev)) {
> >               qed_vf_get_link_params(hwfn, params);
> >               qed_vf_get_link_state(hwfn, link);
> >               qed_vf_get_link_caps(hwfn, link_caps); diff --git
> > a/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> > b/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> > index c8667c65e685..c90b2b6ad969 100644
> > --- a/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> > +++ b/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> > @@ -12,11 +12,13 @@
> >  #include "qed_vf.h"
> >  #define QED_VF_ARRAY_LENGTH (3)
> > 
> > +#ifdef CONFIG_QED_SRIOV
> >  #define IS_VF(cdev)             ((cdev)->b_is_vf)
> >  #define IS_PF(cdev)             (!((cdev)->b_is_vf))
> > -#ifdef CONFIG_QED_SRIOV
> >  #define IS_PF_SRIOV(p_hwfn)     (!!((p_hwfn)->cdev->p_iov_info))
> >  #else
> > +#define IS_VF(cdev)             (0)
> > +#define IS_PF(cdev)             (1)
> >  #define IS_PF_SRIOV(p_hwfn)     (0)
> >  #endif
> >  #define IS_PF_SRIOV_ALLOC(p_hwfn)       (!!((p_hwfn)->pf_iov_info))
> > 
> > I don't see why that isn't already the case actually. If this is ok, I'll send an
> > updated patch.
> > 
> > For the PF case, we still need to fix the qed_mcp_get_link_params() failure case,
> > so the rest of my patch is needed anyway, regardless of how we address the
> > warning.
> > 
> 
> I think that would be unsafe with current qede - 
> qede currently publishes its VFs' PCI device-id as part its MODULE_DEVICE_TABLE,
> even if CONFIG_QED_SRIOV isn't enabled [might be the wrong thing to do, but that
> how it goes].
> Without changing this, if for some reason we'd have an assigned VF to a VM
> whose kernel isn't compiled with CONFIG_QED_SRIOV [which is an odd config],
> that VM is likely to miserably crash.

Wouldn't it crash anyway if the code to handle VF devices is not present?
E.g. the warning we got here tells us that qed_get_link_data() operates
on uninitialized data when called on a VF device and SRIOV support is
not built into the driver. I haven't looked if all the other functions
handle that right, but my guess is that there are other functions with
similar problems.

Maybe it's best to remove the PCI IDs fort the virtual devices from the
table if they are not supported by the configuration.

	Arnd

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH] qed: fix qed_fill_link() error handling David Miller <davem@davemloft.net> - 2016-05-31 23:30 +0200
  Re: [PATCH] qed: fix qed_fill_link() error handling Arnd Bergmann <arnd@arndb.de> - 2016-06-01 00:40 +0200
    RE: [PATCH] qed: fix qed_fill_link() error handling Yuval Mintz <Yuval.Mintz@qlogic.com> - 2016-06-01 13:00 +0200
      Re: [PATCH] qed: fix qed_fill_link() error handling Arnd Bergmann <arnd@arndb.de> - 2016-06-01 13:10 +0200
        RE: [PATCH] qed: fix qed_fill_link() error handling Yuval Mintz <Yuval.Mintz@qlogic.com> - 2016-06-01 13:20 +0200
          [PATCH v2] qed: fix qed_fill_link() error handling Arnd Bergmann <arnd@arndb.de> - 2016-06-01 15:30 +0200
            RE: [PATCH v2] qed: fix qed_fill_link() error handling Yuval Mintz <Yuval.Mintz@qlogic.com> - 2016-06-01 15:40 +0200
              Re: [PATCH v2] qed: fix qed_fill_link() error handling David Miller <davem@davemloft.net> - 2016-06-02 07:10 +0200

csiph-web