On Tue, Aug 15, 2017 at 11:32:02AM +0800, oulijun wrote:
> 在 2017/8/13 18:18, Leon Romanovsky 写道:
> > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> > The functions which are not implemented can be simply ignored
> > instead of defining empty function. This patch removes such functions
> > from hns driver.
> >
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > Reviewed-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxx>
> > ---
> > drivers/infiniband/hw/hns/hns_roce_main.c | 14 --------------
> > 1 file changed, 14 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c b/drivers/infiniband/hw/hns/hns_roce_main.c
> > index d9777b662eba..250e2059ef07 100644
> > --- a/drivers/infiniband/hw/hns/hns_roce_main.c
> > +++ b/drivers/infiniband/hw/hns/hns_roce_main.c
> > @@ -285,12 +285,6 @@ static enum rdma_link_layer hns_roce_get_link_layer(struct ib_device *device,
> > return IB_LINK_LAYER_ETHERNET;
> > }
> >
> > -static int hns_roce_query_gid(struct ib_device *ib_dev, u8 port_num, int index,
> > - union ib_gid *gid)
> > -{
> > - return 0;
> > -}
> > -
> > static int hns_roce_query_pkey(struct ib_device *ib_dev, u8 port, u16 index,
> > u16 *pkey)
> > {
> > @@ -316,12 +310,6 @@ static int hns_roce_modify_device(struct ib_device *ib_dev, int mask,
> > return 0;
> > }
> >
> > -static int hns_roce_modify_port(struct ib_device *ib_dev, u8 port_num, int mask,
> > - struct ib_port_modify *props)
> > -{
> > - return 0;
> > -}
> > -
> > static struct ib_ucontext *hns_roce_alloc_ucontext(struct ib_device *ib_dev,
> > struct ib_udata *udata)
> > {
> > @@ -462,10 +450,8 @@ static int hns_roce_register_device(struct hns_roce_dev *hr_dev)
> > ib_dev->modify_device = hns_roce_modify_device;
> > ib_dev->query_device = hns_roce_query_device;
> > ib_dev->query_port = hns_roce_query_port;
> > - ib_dev->modify_port = hns_roce_modify_port;
> > ib_dev->get_link_layer = hns_roce_get_link_layer;
> > ib_dev->get_netdev = hns_roce_get_netdev;
> > - ib_dev->query_gid = hns_roce_query_gid;
> > ib_dev->add_gid = hns_roce_add_gid;
> > ib_dev->del_gid = hns_roce_del_gid;
> > ib_dev->query_pkey = hns_roce_query_pkey;
>
> Hi, Leon
>
> I have test this patch on D05 board used 4.13 rc-0 kernel and the driver load fail.
>
> The log as follows:
>
> [ 31.858931] sdhci: Secure Digital Host Controller Interface driver
> [ 31.865179] sdhci: Copyright(c) Pierre Ossman
> [ 31.869631] Synopsys Designware Multimedia Card Interface Driver
> [ 31.875819] sdhci-pltfm: SDHCI platform and OF driver helper
> [ 31.882683] ledtrig-cpu: registered to indicate activity on CPUs
> [ 31.976315] Device hns_0 is missing mandatory function query_gid
> [ 31.982436] hns_roce HISI00D1:00: ib_register_device failed!
> [ 31.989561] usbcore: registered new interface driver usbhid
> [ 31.995249] usbhid: USB HID core driver
> [ 32.000197] NET: Registered protocol family 10
> [ 32.005730] Segment Routing with IPv6
> [ 32.009490] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
> [ 32.015734] NET: Registered protocol family 17
>
>
> From the above log, I have analysed the quetion. Firstly, I think that the query_gid function is not
>
> be called and used because the IB core has do it. However, I think that it should be reserved it
>
> and direct return because it need check the device's mandatory. The interface implemented
>
> as follows:
>
> static int ib_device_check_mandatory(struct ib_device *device)
> {
> #define IB_MANDATORY_FUNC(x) { offsetof(struct ib_device, x), #x }
> static const struct {
> size_t offset;
> char *name;
> } mandatory_table[] = {
> IB_MANDATORY_FUNC(query_device),
> IB_MANDATORY_FUNC(query_port),
> IB_MANDATORY_FUNC(query_pkey),
> IB_MANDATORY_FUNC(query_gid),
> IB_MANDATORY_FUNC(alloc_pd),
> IB_MANDATORY_FUNC(dealloc_pd),
> IB_MANDATORY_FUNC(create_ah),
> IB_MANDATORY_FUNC(destroy_ah),
> IB_MANDATORY_FUNC(create_qp),
> IB_MANDATORY_FUNC(modify_qp),
> IB_MANDATORY_FUNC(destroy_qp),
> IB_MANDATORY_FUNC(post_send),
> IB_MANDATORY_FUNC(post_recv),
> IB_MANDATORY_FUNC(create_cq),
> IB_MANDATORY_FUNC(destroy_cq),
> IB_MANDATORY_FUNC(poll_cq),
> IB_MANDATORY_FUNC(req_notify_cq),
> IB_MANDATORY_FUNC(get_dma_mr),
> IB_MANDATORY_FUNC(dereg_mr),
> IB_MANDATORY_FUNC(get_port_immutable)
> };
> int i;
>
> for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) {
> if (!*(void **) ((void *) device + mandatory_table[i].offset)) {
> pr_warn("Device %s is missing mandatory function %s\n",
> device->name, mandatory_table[i].name);
> return -EINVAL;
> }
> }
>
> return 0;
> }
>
> In conclusion, I think that this patch may result in a bug for hns_roce driver. Are your suggestion?
>
The patch below can resolve your failure, but the warning will be fired
anyway, which is not clean as I would like to see.
[ 31.976315] Device hns_0 is missing mandatory function query_gid
To look after solution, I compared the "InfiniBand Architecture Release 1.3"
and A"nnex A (RoCE)" and it looks like the query_gid is mandatory in IB (part of
query HCA) and isn't required for RoCE.
I have prototype code to move RDMA core to use static ndo type of
initialization per supported mode (iWARP, RoCE, IB) instead of dynamic
assignments as it is done now, but unfortunately it is not ready for
prime time yet.
In the meantime, I'll drop this patch.
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 475b93d62748..878760a858db 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -853,7 +853,10 @@ int ib_query_gid(struct ib_device *device,
if (attr)
return -EINVAL;
- return device->query_gid(device, port_num, index, gid);
+ if (device->query_gid)
+ return device->query_gid(device, port_num, index, gid);
+
+ return 0;
}
EXPORT_SYMBOL(ib_query_gid);
Thank you for your excellent analysis and your report.
>
> Thanks
>
> Lijun Ou
>
>
Attachment:
signature.asc
Description: PGP signature
