netdev - Re: [PATCH net 3/3] i2c: designware: support hardware lock for Wangxun 10Gb NIC

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: <ZsiZALjnoUpb0H_I@smile.fi.intel.com>
Date: Fri, 23 Aug 2024 17:13:20 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: andi.shyti@...nel.org, jarkko.nikula@...ux.intel.com,
	mika.westerberg@...ux.intel.com, jsd@...ihalf.com,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, rmk+kernel@...linux.org.uk,
	piotr.raczynski@...el.com, andrew@...n.ch,
	linux-i2c@...r.kernel.org, netdev@...r.kernel.org,
	mengyuanlou@...-swift.com, duanqiangwen@...-swift.com,
	stable@...r.kernel.org
Subject: Re: [PATCH net 3/3] i2c: designware: support hardware lock for
 Wangxun 10Gb NIC

On Fri, Aug 23, 2024 at 11:02:42AM +0800, Jiawen Wu wrote:
> Support acquire_lock() and release_lock() for Wangxun 10Gb NIC. Since the
> firmware needs to access I2C all the time for some features, the semaphore
> is used between software and firmware. The driver should set software
> semaphore before accessing I2C bus and release it when it is finished.
> Otherwise, there is probability that the correct information on I2C bus
> will not be obtained.

...

>  i2c-designware-core-$(CONFIG_I2C_DESIGNWARE_SLAVE) 	+= i2c-designware-slave.o

>  i2c-designware-platform-y 				:= i2c-designware-platdrv.o
> +i2c-designware-platform-y 				+= i2c-designware-wx.o

These lines have TABs/spaces mixture. Please fix at least your entry to avoid
this from happening.


...

>  int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev);
>  #endif

^^^

> +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev);

See below.

...

>  		.probe = i2c_dw_amdpsp_probe_lock_support,
>  	},
>  #endif

^^^

> +	{
> +		.probe = i2c_dw_txgbe_probe_lock_support,
> +	},

Do we all need this support? Even if the driver is not compiled? Why?

...

> +#include <linux/platform_data/i2c-wx.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/pci.h>

This is a semi-random list. Please, take your time to understand the core you
wrote. Follow IWYU principle.

...

> +static int i2c_dw_txgbe_acquire_lock(struct dw_i2c_dev *dev)
> +{
> +	void __iomem *req_addr;
> +	u32 swsm;
> +	int i;
> +
> +	req_addr = dev->ext + I2C_DW_TXGBE_MNG_SW;
> +
> +	for (i = 0; i < I2C_DW_TXGBE_REQ_RETRY_CNT; i++) {

Retry loops much better in a form of

	unsigned int retries = ...;
	...
	do {
		...
	} while (--retries);

BUT... see below.

> +		writel(I2C_DW_TXGBE_MNG_SW_SM, req_addr);
> +
> +		/* If we set the bit successfully then we got semaphore. */
> +		swsm = readl(req_addr);
> +		if (swsm & I2C_DW_TXGBE_MNG_SW_SM)
> +			break;
> +
> +		udelay(50);

So, can a macro from iopoll.h be utilised here? Why not?

> +	}
> +
> +	if (i == I2C_DW_TXGBE_REQ_RETRY_CNT)
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}

> +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev->dev);

Why do you need this dance? I.o.w. how pdev is being used here?

> +	struct txgbe_i2c_platform_data *pdata;
> +
> +	pdata = dev_get_platdata(&pdev->dev);
> +	if (!pdata)
> +		return -ENXIO;
> +
> +	dev->ext = pdata->hw_addr;
> +	if (!dev->ext)
> +		return -ENXIO;
> +
> +	dev->acquire_lock = i2c_dw_txgbe_acquire_lock;
> +	dev->release_lock = i2c_dw_txgbe_release_lock;
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko