linux-kernel - Re: [PATCH -next 2/2] i2c: designware: Add support for new SoCs in AMDPSP driver

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]

Date:   Mon, 19 Sep 2022 16:59:51 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Jan Dabros <jsd@...ihalf.com>
Cc:     linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
        jarkko.nikula@...ux.intel.com, wsa@...nel.org,
        rrangel@...omium.org, upstream@...ihalf.com,
        mario.limonciello@....com
Subject: Re: [PATCH -next 2/2] i2c: designware: Add support for new SoCs in
 AMDPSP driver

On Fri, Sep 16, 2022 at 03:18:54PM +0200, Jan Dabros wrote:
> New AMD SoCs are using different algorithm for x86-PSP communication,
> thus need to modify I2C arbitration driver. Since possible future
> revisions should have follow new approach, mark functions used only for
> Cezanne with "czn_" prefix.
> 
> While at it, remove redundant check by modifying psp_wait_cmd() to only
> check for MBOX_READY bit, since MBOX_CMD field being zero is verified by
> czn_psp_check_mbox_sts() later on in the sequence.

...

> -#define PSP_MBOX_CMD_OFFSET		0x3810570
> -#define PSP_MBOX_BUFFER_L_OFFSET	0x3810574
> -#define PSP_MBOX_BUFFER_H_OFFSET	0x3810578
> +#define CZN_PSP_MBOX_CMD_OFFSET		0x3810570
> +#define CZN_PSP_MBOX_BUFFER_L_OFFSET	0x3810574
> +#define CZN_PSP_MBOX_BUFFER_H_OFFSET	0x3810578

Can we avoid this renaming noise by putting the proper names in the first place
(in the first patch)? Respectively move the corresponding commit message piece
to there.

Or looking into the below maybe even split the renaming to a separate
non-functional change?

...

> +	if (req)
> +		ret = czn_psp_send_cmd(req);
> +	else
> +		ret = psp_send_cmd(i2c_req_type);

> +

Unnecessary blank line.

> +	if (ret)
>  		return -EIO;

Why you can't return the actual error code here?

...

>  	start = jiffies;
>  	ret = read_poll_timeout(psp_send_check_i2c_req, status,
>  				(status != -EBUSY),
>  				PSP_I2C_REQ_RETRY_DELAY_US,
>  				PSP_I2C_REQ_RETRY_CNT * PSP_I2C_REQ_RETRY_DELAY_US,
> -				0, req);
> +				0, req, i2c_req_type);

> +

Stray blank line addition.

>  	if (ret) {
>  		dev_err(psp_i2c_dev, "Timed out waiting for PSP to %s I2C bus\n",
>  			(i2c_req_type == PSP_I2C_REQ_ACQUIRE) ?

-- 
With Best Regards,
Andy Shevchenko