linux-kernel - Re: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer

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] [day] [month] [year] [list]

Message-ID: <e12dea50-5fae-c31c-46e6-572488ca9ca1@alliedtelesis.co.nz>
Date:   Tue, 13 Apr 2021 23:23:45 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Wolfram Sang <wsa@...nel.org>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer


On 14/04/21 10:28 am, Chris Packham wrote:
>
> On 14/04/21 1:52 am, Andy Shevchenko wrote:
>> On Tue, Apr 13, 2021 at 8:10 AM Chris Packham
>> <chris.packham@...iedtelesis.co.nz> wrote:
>>> The fsl-i2c controller will generate an interrupt after every byte
>>> transferred. Make use of this interrupt to drive a state machine which
>>> allows the next part of a transfer to happen as soon as the 
>>> interrupt is
>>> received. This is particularly helpful with SMBUS devices like the LM81
>>> which will timeout if we take too long between bytes in a transfer.
>> Also see my other comments below.
>>
>> ...
>>
...
>>
>>> +               dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
>>> +                       action_str[i2c->action], byte);
>> You already printed action. Anything changed?
> It's mainly the addition of the byte read. I couldn't figure out a 
> sensible way of always printing the action then appending the data in 
> the read/write case. Open to suggestions.
>>
>>> +               dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
>>> +                       action_str[i2c->action], 
>>> msg->buf[i2c->byte_posn]);
>> Deduplicate this. Perhaps at the end of switch-case print once with
>> whatever temporary variable value you want to.
>>
>> ...
> I thought about this but decided not to because in the write case it's 
> printed before going to hardware and in the read case it's after. If I 
> moved it after the case I'd have to use something other than 
> i2c->byte_posn which seemed error prone.
I looked at a few options for this. Avoiding repeating the action is 
doable but I feel it needs to be replaced by something otherwise it's 
just a random byte value in the output (e.g. "buf = "/"byte = "). 
Rolling everything into a single line of output seems really hard to do 
to cover all the possible actions.

Completely deduplicating this means I need to add code to store the 
action before the case and check it afterward which will run all the 
time. This seems overkill for the sake of avoiding duplicate code which 
is usually compiled out.

I'm erring on the side of just removing __func__ and leaving the rest 
as-is. Unless you feel really strongly that something else should be done.

One option not mentioned is to remove these two debug statements. I'd be 
OK with that.