linux-kernel - Re: [PATCH v4 3/5] i2c:ocores: add polling interface

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, 11 Feb 2019 13:35:15 +0000
From:   Peter Rosin <peda@...ntia.se>
To:     "federico.vaga@...n.ch" <federico.vaga@...n.ch>
CC:     Peter Korsgaard <peter@...sgaard.com>,
        Andrew Lunn <andrew@...n.ch>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/5] i2c:ocores: add polling interface

>>> @@ -294,7 +427,7 @@ static int ocores_init(struct device *dev, struct
>>> ocores_i2c *i2c)
>>
>>>  
>>>  
>>>  	/* Init the device */
>>>  	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
>>>
>>> -	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
>>> +	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN);
>>
>>
>> You fix this up in patch 5 (in what is perhaps carelessly marketed as
>> fixes for minor checkpatch issues), but for this patch you are actually
>> no longer making sure that you clear the OCI2C_CTRL_IEN bit as the code
>> used to before this patch. I think you intended to preserve that
>> behavior, no?
> 
> I think you got confused by the two patches because those lines look very 
> similar.
> 
> In PATCH 5 what you see is the "style fix" to clear EN and IEN before clock 
> configuration
> 
> in PATCH 3 (this one) what you see is when later (same function, after clock 
> configuration) the device is re-enabled (EN), but without enabling the 
> interrupt because on polling configuration we do not want to see interrupt 
> flowing.
> 
> So, the behavior is preserved for what concern clearing the IEN bit before 
> clock configuration.
> 
> am I wrong?

No, I don't think I'm confused and I think you are wrong. Current code does
this:

	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);

	/* make sure the device is disabled */
	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_IEN|OCI2C_CTRL_EN));
	...
	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN));

Here, the final oc_setreg always sets OCI2C_CTRL_IEN.


Patch 3 changes it to:

	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);

	/* make sure the device is disabled */
	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_IEN|OCI2C_CTRL_EN));
	...
	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN));

Here, the final oc_setreg restores OCI2C_CTRL_IEN to whatever it was
at function entry.


And patch 5 changes it again to:

	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);

	/* make sure the device is disabled */
	ctrl &= ~(OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
	oc_setreg(i2c, OCI2C_CONTROL, ctrl);
	...
	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN));

Here, the final oc_setreg keeps OCI2C_CTRL_IEN cleared. I think you wanted
this deterministic behavior for patch 3 as well.

Cheers,
Peter