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

[Arnd Bergmann]

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