lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <MN2PR12MB297560E219744B3B1E2A768FC76B9@MN2PR12MB2975.namprd12.prod.outlook.com>
Date:   Tue, 16 Mar 2021 23:28:28 +0000
From:   David Thompson <davthompson@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        Liming Sun <limings@...dia.com>,
        Asmaa Mnebhi <asmaa@...dia.com>
Subject: RE: [PATCH net-next v4] Add Mellanox BlueField Gigabit Ethernet
 driver

> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Thursday, March 11, 2021 9:20 PM
> To: David Thompson <davthompson@...dia.com>
> Cc: netdev@...r.kernel.org; davem@...emloft.net; kuba@...nel.org; Liming
> Sun <limings@...dia.com>; Asmaa Mnebhi <asmaa@...dia.com>
> Subject: Re: [PATCH net-next v4] Add Mellanox BlueField Gigabit Ethernet driver
> 
> > +#define DRV_VERSION 1.19
> 
> > +static int mlxbf_gige_probe(struct platform_device *pdev) {
> > +	unsigned int phy_int_gpio;
> > +	struct phy_device *phydev;
> > +	struct net_device *netdev;
> > +	struct resource *mac_res;
> > +	struct resource *llu_res;
> > +	struct resource *plu_res;
> > +	struct mlxbf_gige *priv;
> > +	void __iomem *llu_base;
> > +	void __iomem *plu_base;
> > +	void __iomem *base;
> > +	int addr, version;
> > +	u64 control;
> > +	int err = 0;
> > +
> > +	if (device_property_read_u32(&pdev->dev, "version", &version)) {
> > +		dev_err(&pdev->dev, "Version Info not found\n");
> > +		return -EINVAL;
> > +	}
> 
> Is this a device tree property? ACPI? If it is device tree property you need to
> document the binding, Documentation/devicetree/bindinds/...
> 

This driver gets its properties from an ACPI table, not from device tree.
The "version" property is read from the ACPI table, and if an incompatible
table version is found the driver does not load.  This logic allows the version
of the driver and the version of the ACPI table to change and compatibility
is ensured.  If there's a different/better way to do this, please let me know.

> > +
> > +	if (version != (int)DRV_VERSION) {
> > +		dev_err(&pdev->dev, "Version Mismatch. Expected %d
> Returned %d\n",
> > +			(int)DRV_VERSION, version);
> > +		return -EINVAL;
> > +	}
> 
> That is odd. Doubt odd. First of, why (int)1.19? Why not just set DRV_VERSION
> to 1? This is the only place you use this, so the .19 seems pointless. Secondly,
> what does this version in DT/ACPI actually represent? The hardware version?
> Then you should be using a compatible string? Or read a hardware register which
> tells you have hardware version.
> 

The value of DRV_VERSION is 1.19 because it specifies the <major>.<minor>
version of the driver.  This value is used by the MODULE_VERSION() macro.
During the review of a prior patch version I was told to remove the use of
the MODULE_VERSION() macro, so it is not shown here in this review.  Please
let me know if the removal of MODULE_VERSION() is advised.   There could 
be a different #define, e.g. "ACPI_TABLE_VERSION 1" instead of overloading
the DRV_VERSION via the (int) cast.  That would probably make more sense.

> > +
> > +	err = device_property_read_u32(&pdev->dev, "phy-int-gpio",
> &phy_int_gpio);
> > +	if (err < 0)
> > +		phy_int_gpio = MLXBF_GIGE_DEFAULT_PHY_INT_GPIO;
> 
> Again, this probably needs documenting. This is not how you do interrupts with
> DT. I also don't think it is correct for ACPI, but i don't know ACPI.
> 

Right, the "phy-int-gpio" is a property read from the ACPI table.
Is there a convention for ACPI table use/documentation?

> > +	phydev = phy_find_first(priv->mdiobus);
> > +	if (!phydev) {
> > +		mlxbf_gige_mdio_remove(priv);
> > +		return -ENODEV;
> > +	}
> 
> If you are using DT, please use a phandle to the device on the MDIO bus.
> 

The code is not using DT.

> > +	/* Sets netdev->phydev to phydev; which will eventually
> > +	 * be used in ioctl calls.
> > +	 * Cannot pass NULL handler.
> > +	 */
> > +	err = phy_connect_direct(netdev, phydev,
> > +				 mlxbf_gige_adjust_link,
> > +				 PHY_INTERFACE_MODE_GMII);
> 
> It does a lot more than just set netdev->phydev. I'm not sure this comment has
> any real value.
> 
> 	Andrew

Yes, good point.  This comment will be updated for completeness or removed.

Regards,
Dave

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ