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

[Yuval Mintz]

> > > 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
> >
> > 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.

Actually, I think VF probe should gracefully fail in that case,
as qed_vf_hw_prepare() would simply return -EINVAL.
But I can honestly say I've never tested this flow, and I agree there's
no reason to allow VF probe in case we're not supporting SRIOV.

So I guess removing the PCI ID and defining IS_PF to be true in case
CONFIG_QED_SRIOV isn't set is the right way to go.
Do you want to revise your patch, or do you want me to do it?

Thanks,
Yuval

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

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

[Yuval Mintz]

> > 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.
> 
>   Arnd

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.

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

[Arnd Bergmann]

On Tuesday, May 31, 2016 2:20:46 PM CEST David Miller wrote:
> From: Yuval Mintz 
> Date: Mon, 30 May 2016 16:24:07 +
> 
> >> +if (IS_ENABLED(CONFIG_QED_SRIOV) && !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);
> >> +
> >> +return 0;
> >> +}
> > 
> > The IS_ENABLED here seems a bit wasteful to me - we have empty 
> > implementation
> > under qed_vf.h just for this case [I.e., that SRIOV isn't enabled for qed].
> > If all we're trying achieve is removing these gcc warnings, I think we can 
> > simply
> > memset the structs in the currently-empty qed_vf_get_link_* functions.

Adding a memset() to those functions would add a bit of overhead in code size
because that ends up being unused in practice without a way for the compiler
to know, I added the IS_ENABLED() check to reduce the object code size here
by also eliminating the check for IS_PF().

> 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.

Arnd

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

[David Miller]

From: Yuval Mintz 
Date: Mon, 30 May 2016 16:24:07 +

>> +if (IS_ENABLED(CONFIG_QED_SRIOV) && !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);
>> +
>> +return 0;
>> +}
> 
> The IS_ENABLED here seems a bit wasteful to me - we have empty implementation
> under qed_vf.h just for this case [I.e., that SRIOV isn't enabled for qed].
> If all we're trying achieve is removing these gcc warnings, I think we can 
> simply
> memset the structs in the currently-empty qed_vf_get_link_* functions.

I think both solutions are equally valid/elegant.

Arnd?

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

[Yuval Mintz]

> + if (IS_ENABLED(CONFIG_QED_SRIOV) && !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);
> +
> + return 0;
> + }

The IS_ENABLED here seems a bit wasteful to me - we have empty implementation
under qed_vf.h just for this case [I.e., that SRIOV isn't enabled for qed].
If all we're trying achieve is removing these gcc warnings, I think we can 
simply
memset the structs in the currently-empty qed_vf_get_link_* functions.

[PATCH] qed: fix qed_fill_link() error handling

[Arnd Bergmann]

gcc warns about qed_fill_link possibly accessing uninitialized data:

drivers/net/ethernet/qlogic/qed/qed_main.c: In function 'qed_fill_link':
drivers/net/ethernet/qlogic/qed/qed_main.c:1170:35: error: 'link_caps' may be 
used uninitialized in this function [-Werror=maybe-uninitialized]

While this warning is only about the specific case of CONFIG_QED_SRIOV
being disabled but the function getting called for a VF (which should
never happen), another possibility is that qed_mcp_get_*() fails without
returning data.

This rearranges the code so we bail out in either of the two cases
and print a warning instead of accessing the uninitialized data.

The qed_link_output structure remains untouched in this case, but
all callers first call memset() on it, so at least we are not leaking
stack data then.

Signed-off-by: Arnd Bergmann 
---
 drivers/net/ethernet/qlogic/qed/qed_main.c | 45 --
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c 
b/drivers/net/ethernet/qlogic/qed/qed_main.c
index 753064679bde..68f87a4f9316 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_main.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
@@ -1105,6 +1105,39 @@ static int qed_get_port_type(u32 media_type)
return port_type;
 }
 
+static int qed_get_link_data(struct qed_hwfn *hwfn,
+struct qed_mcp_link_params *params,
+struct qed_mcp_link_state *link,
+struct qed_mcp_link_capabilities *link_caps)
+{
+   void *p;
+
+   if (IS_ENABLED(CONFIG_QED_SRIOV) && !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);
+
+   return 0;
+   }
+
+   p = qed_mcp_get_link_params(hwfn);
+   if (!p)
+   return -ENXIO;
+   memcpy(params, p, sizeof(*params));
+
+   p = qed_mcp_get_link_state(hwfn);
+   if (!p)
+   return -ENXIO;
+   memcpy(link, p, sizeof(*link));
+
+   p = qed_mcp_get_link_capabilities(hwfn);
+   if (!p)
+   return -ENXIO;
+   memcpy(link_caps, p, sizeof(*link_caps));
+
+   return 0;
+}
+
 static void qed_fill_link(struct qed_hwfn *hwfn,
  struct qed_link_output *if_link)
 {
@@ -1116,15 +1149,9 @@ static void qed_fill_link(struct qed_hwfn *hwfn,
memset(if_link, 0, sizeof(*if_link));
 
/* Prepare source inputs */
-   if (IS_PF(hwfn->cdev)) {
-   memcpy(, qed_mcp_get_link_params(hwfn), sizeof(params));
-   memcpy(, qed_mcp_get_link_state(hwfn), sizeof(link));
-   memcpy(_caps, qed_mcp_get_link_capabilities(hwfn),
-  sizeof(link_caps));
-   } else {
-   qed_vf_get_link_params(hwfn, );
-   qed_vf_get_link_state(hwfn, );
-   qed_vf_get_link_caps(hwfn, _caps);
+   if (qed_get_link_data(hwfn, , , _caps)) {
+   dev_warn(>cdev->pdev->dev, "no link data available\n");
+   return;
}
 
/* Set the link parameters to pass to protocol driver */
-- 
2.7.0