Re: [PATCH for-next 2/2] RDMA/efa: Report host information to the device

[Leon Romanovsky <leon@kernel.org>]

On Sun, May 10, 2020 at 04:05:45PM +0300, Gal Pressman wrote:
> On 10/05/2020 15:29, Leon Romanovsky wrote:
> > On Sun, May 10, 2020 at 02:59:18PM +0300, Gal Pressman wrote:
> >> diff --git a/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h b/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
> >> index 96b104ab5415..efdeebc9ea9b 100644
> >> --- a/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
> >> +++ b/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
> >> @@ -37,7 +37,7 @@ enum efa_admin_aq_feature_id {
> >>  	EFA_ADMIN_NETWORK_ATTR                      = 3,
> >>  	EFA_ADMIN_QUEUE_ATTR                        = 4,
> >>  	EFA_ADMIN_HW_HINTS                          = 5,
> >> -	EFA_ADMIN_FEATURES_OPCODE_NUM               = 8,
> >> +	EFA_ADMIN_HOST_INFO                         = 6,
> >>  };
> >>
> >>  /* QP transport type */
> >> @@ -799,6 +799,55 @@ struct efa_admin_mmio_req_read_less_resp {
> >>  	u32 reg_val;
> >>  };
> >>
> >> +enum efa_admin_os_type {
> >> +	EFA_ADMIN_OS_LINUX                          = 0,
> >> +	EFA_ADMIN_OS_WINDOWS                        = 1,
> >
> > Not used.
>
> That's the device interface..

It doesn't matter, we don't add code/defines that are not in use.

>
> >
> >> +};
> >> +
> >> +struct efa_admin_host_info {
> >> +	/* OS distribution string format */
> >> +	u8 os_dist_str[128];
> >> +
> >> +	/* Defined in enum efa_admin_os_type */
> >> +	u32 os_type;
> >> +
> >> +	/* Kernel version string format */
> >> +	u8 kernel_ver_str[32];
> >> +
> >> +	/* Kernel version numeric format */
> >> +	u32 kernel_ver;
> >> +
> >> +	/*
> >> +	 * 7:0 : driver_module_type
> >> +	 * 15:8 : driver_sub_minor
> >> +	 * 23:16 : driver_minor
> >> +	 * 31:24 : driver_major
> >> +	 */
> >> +	u32 driver_ver;
> >
> > No to this.
>
> Same, this is the device interface.
> And obviously it's not used as we don't have a driver version.
>
> >
> >> +
> >> +	/*
> >> +	 * Device's Bus, Device and Function
> >> +	 * 2:0 : function
> >> +	 * 7:3 : device
> >> +	 * 15:8 : bus
> >> +	 */
> >> +	u16 bdf;
> >> +
> >> +	/*
> >> +	 * Spec version
> >> +	 * 7:0 : spec_minor
> >> +	 * 15:8 : spec_major
> >> +	 */
> >> +	u16 spec_ver;
> >> +
> >> +	/*
> >> +	 * 0 : intree - Intree driver
> >> +	 * 1 : gdr - GPUDirect RDMA supported
> >> +	 * 31:2 : reserved2
> >> +	 */
> >> +	u32 flags;
> >> +};
> >> +
> >>  /* create_qp_cmd */
> >>  #define EFA_ADMIN_CREATE_QP_CMD_SQ_VIRT_MASK                BIT(0)
> >>  #define EFA_ADMIN_CREATE_QP_CMD_RQ_VIRT_MASK                BIT(1)
> >> @@ -820,4 +869,17 @@ struct efa_admin_mmio_req_read_less_resp {
> >>  /* feature_device_attr_desc */
> >>  #define EFA_ADMIN_FEATURE_DEVICE_ATTR_DESC_RDMA_READ_MASK   BIT(0)
> >>
> >> +/* host_info */
> >> +#define EFA_ADMIN_HOST_INFO_DRIVER_MODULE_TYPE_MASK         GENMASK(7, 0)
> >> +#define EFA_ADMIN_HOST_INFO_DRIVER_SUB_MINOR_MASK           GENMASK(15, 8)
> >> +#define EFA_ADMIN_HOST_INFO_DRIVER_MINOR_MASK               GENMASK(23, 16)
> >> +#define EFA_ADMIN_HOST_INFO_DRIVER_MAJOR_MASK               GENMASK(31, 24)
> >
> > Not in use.
>
> Same.

Same :)

>
> >
> >> +#define EFA_ADMIN_HOST_INFO_FUNCTION_MASK                   GENMASK(2, 0)
> >> +#define EFA_ADMIN_HOST_INFO_DEVICE_MASK                     GENMASK(7, 3)
> >> +#define EFA_ADMIN_HOST_INFO_BUS_MASK                        GENMASK(15, 8)
> >> +#define EFA_ADMIN_HOST_INFO_SPEC_MINOR_MASK                 GENMASK(7, 0)
> >> +#define EFA_ADMIN_HOST_INFO_SPEC_MAJOR_MASK                 GENMASK(15, 8)
> >> +#define EFA_ADMIN_HOST_INFO_INTREE_MASK                     BIT(0)
> >> +#define EFA_ADMIN_HOST_INFO_GDR_MASK                        BIT(1)
> >> +
> >>  #endif /* _EFA_ADMIN_CMDS_H_ */
> >> +static void efa_set_host_info(struct efa_dev *dev)
> >> +{
> >> +	struct efa_admin_set_feature_resp resp = {};
> >> +	struct efa_admin_set_feature_cmd cmd = {};
> >> +	struct efa_admin_host_info *hinf;
> >> +	u32 bufsz = sizeof(*hinf);
> >> +	dma_addr_t hinf_dma;
> >> +
> >> +	if (!efa_com_check_supported_feature_id(&dev->edev,
> >> +						EFA_ADMIN_HOST_INFO))
> >> +		return;
> >> +
> >> +	/* Failures in host info set shall not disturb probe */
> >> +	hinf = dma_alloc_coherent(&dev->pdev->dev, bufsz, &hinf_dma,
> >> +				  GFP_KERNEL);
> >> +	if (!hinf)
> >> +		return;
> >> +
> >> +	strlcpy(hinf->os_dist_str, utsname()->release,
> >> +		min(sizeof(hinf->os_dist_str), sizeof(utsname()->release)));
> >> +	hinf->os_type = EFA_ADMIN_OS_LINUX;
> >> +	strlcpy(hinf->kernel_ver_str, utsname()->version,
> >> +		min(sizeof(hinf->kernel_ver_str), sizeof(utsname()->version)));
> >> +	hinf->kernel_ver = LINUX_VERSION_CODE;
> >> +	EFA_SET(&hinf->bdf, EFA_ADMIN_HOST_INFO_BUS, dev->pdev->bus->number);
> >> +	EFA_SET(&hinf->bdf, EFA_ADMIN_HOST_INFO_DEVICE,
> >> +		PCI_SLOT(dev->pdev->devfn));
> >> +	EFA_SET(&hinf->bdf, EFA_ADMIN_HOST_INFO_FUNCTION,
> >> +		PCI_FUNC(dev->pdev->devfn));
> >> +	EFA_SET(&hinf->spec_ver, EFA_ADMIN_HOST_INFO_SPEC_MAJOR,
> >> +		EFA_COMMON_SPEC_VERSION_MAJOR);
> >> +	EFA_SET(&hinf->spec_ver, EFA_ADMIN_HOST_INFO_SPEC_MINOR,
> >> +		EFA_COMMON_SPEC_VERSION_MINOR);
> >> +	EFA_SET(&hinf->flags, EFA_ADMIN_HOST_INFO_INTREE, 1);
> >
> > Ohhh, so users will change this line voluntarily?
>
> Are you worried with out of tree users?

Should I? I'm not worried, but excited to see such naive debug approach.

Thanks