Re: [PATCH v4 06/19] IB/core: Add max_mad_size to ib_device_attr
On Wed, Feb 25, 2015 at 06:32:27PM +, Weiny, Ira wrote:
> > >
> > > Fixed for v5 with this message.
> > >
> > > + dev_err(&device->dev,
> > > + "Max supported MAD size (%u) < min required by
> > > ib_mad
> > (%u), ignoring device (%s)\n",
> > > + device->cached_dev_attrs.max_mad_size,
> > > + IB_MGMT_MAD_SIZE, device->name);
> >
> > It also seems redundant since the only call to ib_mad_port_open is:
> >
> > if (ib_mad_port_open(device, i)) {
> > printk(KERN_ERR PFX "Couldn't open %s port %d\n",
> >device->name, i);
> >
> > So, why does this particular error deserve a special double error print? I
> > assume it is basically impossible to hit?
>
> This does indicate a coding error. Generally I prefer details of
> why the device could not open the port. But if the community feels
> this is redundant or "not possible" I can drop the hunk.
Internal logic errors are handled with WARN_ON/BUG/etc.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 06/19] IB/core: Add max_mad_size to ib_device_attr
> >
> > Fixed for v5 with this message.
> >
> > + dev_err(&device->dev,
> > + "Max supported MAD size (%u) < min required by
> > ib_mad
> (%u), ignoring device (%s)\n",
> > + device->cached_dev_attrs.max_mad_size,
> > + IB_MGMT_MAD_SIZE, device->name);
>
> It also seems redundant since the only call to ib_mad_port_open is:
>
> if (ib_mad_port_open(device, i)) {
> printk(KERN_ERR PFX "Couldn't open %s port %d\n",
>device->name, i);
>
> So, why does this particular error deserve a special double error print? I
> assume it is basically impossible to hit?
This does indicate a coding error. Generally I prefer details of why the
device could not open the port. But if the community feels this is redundant
or "not possible" I can drop the hunk.
Ira
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 06/19] IB/core: Add max_mad_size to ib_device_attr
On Wed, Feb 25, 2015 at 06:13:35PM +, Weiny, Ira wrote:
> > > diff --git a/drivers/infiniband/core/mad.c
> > > b/drivers/infiniband/core/mad.c index 819b794..a6a33cf 100644
> > > +++ b/drivers/infiniband/core/mad.c
> > > @@ -2924,6 +2924,12 @@ static int ib_mad_port_open(struct ib_device
> > *device,
> > > char name[sizeof "ib_mad123"];
> > > int has_smi;
> > >
> > > + if (device->cached_dev_attrs.max_mad_size < IB_MGMT_MAD_SIZE) {
> > > + dev_err(&device->dev, "Min MAD size for device is %u\n",
> > > + IB_MGMT_MAD_SIZE);
> > > + return -EFAULT;
> > > + }
> > > +
> >
> > The printk message here is not very informative and it qualifies as an
> > error.
> > Someone reading that for the first time in the dmesg output and wondering
> > why their device isn't working will be confused if they don't know about the
> > mad size changes you are making here. Something like "max supported MAD
> > size (%u) < min required by ib_mad (%u), ignoring dev \n"
>
> Good suggestion.
>
> Fixed for v5 with this message.
>
> + dev_err(&device->dev,
> + "Max supported MAD size (%u) < min required by ib_mad
> (%u), ignoring device (%s)\n",
> + device->cached_dev_attrs.max_mad_size,
> + IB_MGMT_MAD_SIZE, device->name);
It also seems redundant since the only call to ib_mad_port_open is:
if (ib_mad_port_open(device, i)) {
printk(KERN_ERR PFX "Couldn't open %s port %d\n",
device->name, i);
So, why does this particular error deserve a special double error
print? I assume it is basically impossible to hit?
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 06/19] IB/core: Add max_mad_size to ib_device_attr
> > diff --git a/drivers/infiniband/core/mad.c
> > b/drivers/infiniband/core/mad.c index 819b794..a6a33cf 100644
> > --- a/drivers/infiniband/core/mad.c
> > +++ b/drivers/infiniband/core/mad.c
> > @@ -2924,6 +2924,12 @@ static int ib_mad_port_open(struct ib_device
> *device,
> > char name[sizeof "ib_mad123"];
> > int has_smi;
> >
> > + if (device->cached_dev_attrs.max_mad_size < IB_MGMT_MAD_SIZE) {
> > + dev_err(&device->dev, "Min MAD size for device is %u\n",
> > + IB_MGMT_MAD_SIZE);
> > + return -EFAULT;
> > + }
> > +
>
> The printk message here is not very informative and it qualifies as an error.
> Someone reading that for the first time in the dmesg output and wondering
> why their device isn't working will be confused if they don't know about the
> mad size changes you are making here. Something like "max supported MAD
> size (%u) < min required by ib_mad (%u), ignoring dev \n"
Good suggestion.
Fixed for v5 with this message.
+ dev_err(&device->dev,
+ "Max supported MAD size (%u) < min required by ib_mad
(%u), ignoring device (%s)\n",
+ device->cached_dev_attrs.max_mad_size,
+ IB_MGMT_MAD_SIZE, device->name);
Ira
Re: [PATCH v4 06/19] IB/core: Add max_mad_size to ib_device_attr
On Wed, 2015-02-04 at 18:29 -0500, ira.we...@intel.com wrote:
> From: Ira Weiny
>
> Change all IB drivers to report the max MAD size.
> Add check to verify that all devices support at least IB_MGMT_MAD_SIZE
>
> Signed-off-by: Ira Weiny
>
> ---
>
> Changes from V3:
> Fix ehca compile found with 0-day build
>
> drivers/infiniband/core/mad.c| 6 ++
> drivers/infiniband/hw/amso1100/c2_rnic.c | 1 +
> drivers/infiniband/hw/cxgb3/iwch_provider.c | 1 +
> drivers/infiniband/hw/cxgb4/provider.c | 1 +
> drivers/infiniband/hw/ehca/ehca_hca.c| 3 +++
> drivers/infiniband/hw/ipath/ipath_verbs.c| 1 +
> drivers/infiniband/hw/mlx4/main.c| 1 +
> drivers/infiniband/hw/mlx5/main.c| 1 +
> drivers/infiniband/hw/mthca/mthca_provider.c | 2 ++
> drivers/infiniband/hw/nes/nes_verbs.c| 1 +
> drivers/infiniband/hw/ocrdma/ocrdma_verbs.c | 1 +
> drivers/infiniband/hw/qib/qib_verbs.c| 1 +
> drivers/infiniband/hw/usnic/usnic_ib_verbs.c | 2 ++
> include/rdma/ib_mad.h| 1 +
> include/rdma/ib_verbs.h | 1 +
> 15 files changed, 24 insertions(+)
>
> diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
> index 819b794..a6a33cf 100644
> --- a/drivers/infiniband/core/mad.c
> +++ b/drivers/infiniband/core/mad.c
> @@ -2924,6 +2924,12 @@ static int ib_mad_port_open(struct ib_device *device,
> char name[sizeof "ib_mad123"];
> int has_smi;
>
> + if (device->cached_dev_attrs.max_mad_size < IB_MGMT_MAD_SIZE) {
> + dev_err(&device->dev, "Min MAD size for device is %u\n",
> + IB_MGMT_MAD_SIZE);
> + return -EFAULT;
> + }
> +
The printk message here is not very informative and it qualifies as an
error. Someone reading that for the first time in the dmesg output and
wondering why their device isn't working will be confused if they don't
know about the mad size changes you are making here. Something like
"max supported MAD size (%u) < min required by ib_mad (%u), ignoring dev
\n"
> /* Create new device info */
> port_priv = kzalloc(sizeof *port_priv, GFP_KERNEL);
> if (!port_priv) {
> diff --git a/drivers/infiniband/hw/amso1100/c2_rnic.c
> b/drivers/infiniband/hw/amso1100/c2_rnic.c
> index d2a6d96..63322c0 100644
> --- a/drivers/infiniband/hw/amso1100/c2_rnic.c
> +++ b/drivers/infiniband/hw/amso1100/c2_rnic.c
> @@ -197,6 +197,7 @@ static int c2_rnic_query(struct c2_dev *c2dev, struct
> ib_device_attr *props)
> props->max_srq_sge = 0;
> props->max_pkeys = 0;
> props->local_ca_ack_delay = 0;
> + props->max_mad_size= IB_MGMT_MAD_SIZE;
>
> bail2:
> vq_repbuf_free(c2dev, reply);
> diff --git a/drivers/infiniband/hw/cxgb3/iwch_provider.c
> b/drivers/infiniband/hw/cxgb3/iwch_provider.c
> index 811b24a..b8a80aa0 100644
> --- a/drivers/infiniband/hw/cxgb3/iwch_provider.c
> +++ b/drivers/infiniband/hw/cxgb3/iwch_provider.c
> @@ -1174,6 +1174,7 @@ static int iwch_query_device(struct ib_device *ibdev,
> props->max_pd = dev->attr.max_pds;
> props->local_ca_ack_delay = 0;
> props->max_fast_reg_page_list_len = T3_MAX_FASTREG_DEPTH;
> + props->max_mad_size = IB_MGMT_MAD_SIZE;
>
> return 0;
> }
> diff --git a/drivers/infiniband/hw/cxgb4/provider.c
> b/drivers/infiniband/hw/cxgb4/provider.c
> index 66bd6a2..299c70c 100644
> --- a/drivers/infiniband/hw/cxgb4/provider.c
> +++ b/drivers/infiniband/hw/cxgb4/provider.c
> @@ -332,6 +332,7 @@ static int c4iw_query_device(struct ib_device *ibdev,
> props->max_pd = T4_MAX_NUM_PD;
> props->local_ca_ack_delay = 0;
> props->max_fast_reg_page_list_len = t4_max_fr_depth(use_dsgl);
> + props->max_mad_size = IB_MGMT_MAD_SIZE;
>
> return 0;
> }
> diff --git a/drivers/infiniband/hw/ehca/ehca_hca.c
> b/drivers/infiniband/hw/ehca/ehca_hca.c
> index 9ed4d25..6166146 100644
> --- a/drivers/infiniband/hw/ehca/ehca_hca.c
> +++ b/drivers/infiniband/hw/ehca/ehca_hca.c
> @@ -40,6 +40,7 @@
> */
>
> #include
> +#include
>
> #include "ehca_tools.h"
> #include "ehca_iverbs.h"
> @@ -133,6 +134,8 @@ int ehca_query_device(struct ib_device *ibdev, struct
> ib_device_attr *props)
> if (rblock->hca_cap_indicators & cap_mapping[i + 1])
> props->device_cap_flags |= cap_mapping[i];
>
> + props->max_mad_size = IB_MGMT_MAD_SIZE;
> +
> query_device1:
> ehca_free_fw_ctrlblock(rblock);
>
> diff --git a/drivers/infiniband/hw/ipath/ipath_verbs.c
> b/drivers/infiniband/hw/ipath/ipath_verbs.c
> index 44ea939..4c6474c 100644
> --- a/drivers/infiniband/hw/ipath/ipath_verbs.c
> +++ b/drivers/infiniband/hw/ipath/ipath_verbs.c
> @@ -1538,6 +1538,7 @@ static int ipath_query_device(struct ib_device *ibdev,
> props->max_mcast_qp_attach = ib_ipath_max_mcast_qp_attached;
> props->max_total_mcast_qp_attach = props->max_mcast_q
