linux-kernel - Re: [PATCH v3 5/6] i2c: iproc: handle master read request

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: <1605316659-3422-1-git-send-email-dphadke@linux.microsoft.com>
Date:   Fri, 13 Nov 2020 17:17:39 -0800
From:   Dhananjay Phadke <dphadke@...ux.microsoft.com>
To:     ray.jui@...adcom.com
Cc:     andriy.shevchenko@...ux.intel.com,
        bcm-kernel-feedback-list@...adcom.com, brendanhiggins@...gle.com,
        dphadke@...ux.microsoft.com, f.fainelli@...il.com,
        linux-arm-kernel@...ts.infradead.org, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org, lori.hikichi@...adcom.com,
        rayagonda.kokatanur@...adcom.com, rjui@...adcom.com,
        sbranden@...adcom.com, wsa@...nel.org
Subject: Re: [PATCH v3 5/6] i2c: iproc: handle master read request

On Tue, 10 Nov 2020 11:24:36 -0800, Ray Jui wrote:

>>>> Yes it's true that for master write-read events both
>>>> IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
>>>> So before the slave starts transmitting data to the master, it should
>>>> first read all data from rx-fifo i.e. complete master write and then
>>>> process master read.
>>>>
>>>> To minimise interrupt overhead, we are batching 64bytes.
>>>> To keep isr running for less time, we are using a tasklet.
>>>> Again to keep the tasklet not running for more than 20u, we have set
>>>> max of 10 bytes data read from rx-fifo per tasklet run.
>>>>
>>>> If we start processing everything in isr and using rx threshold
>>>> interrupt, then isr will run for a longer time and this may hog the
>>>> system.
>>>> For example, to process 10 bytes it takes 20us, to process 30 bytes it
>>>> takes 60us and so on.
>>>> So is it okay to run isr for so long ?
>>>>
>>>> Keeping all this in mind we thought a tasklet would be a good option
>>>> and kept max of 10 bytes read per tasklet.
>>>>
>>>> Please let me know if you still feel we should not use a tasklet and
>>>> don't batch 64 bytes.
>>>
>>> Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq)
>>> as i2c rate is quite low.
>>>
>
>kernel thread was proposed in the internal review. I don't see much
>benefit of using tasklet. If a thread is blocked from running for more
>than several tenth of ms, that's really a system-level issue than an
>issue with this driver.
>
>IMO, it's an overkill to use tasklet here but we can probably leave it
>as it is since it does not have a adverse effect and the code ran in
>tasklet is short.
>
>How much time is expected to read 64 bytes from an RX FIFO? Even with
>APB bus each register read is expected to be in the tenth or hundreds of
>nanosecond range. Reading the entire FIFO of 64 bytes should take less
>than 10 us. The interrupt context switch overhead is probably longer
>than that. It's much more effective to read all of them in a single
>batch than breaking them into multiple batches.

OK, there's a general discussions towards removing tasklets, if this
fix works with threaded isr, strongly recommend that route.

This comment in the code suggested that register reads take long time to
drain 64 bytes.

>+/*
>+ * It takes ~18us to reading 10bytes of data, hence to keep tasklet
>+ * running for less time, max slave read per tasklet is set to 10
>bytes.

@Rayagonda, please take care of hand-off mentioned below, once the tasklet
is scheduled, isr should just return and clear status at the end of tasklet.

>>
>> Few other comments -
>>
>>> +              /* schedule tasklet to read data later */
>>> +              tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
>>> +
>>> +              /* clear only IS_S_RX_EVENT_SHIFT interrupt */
>>> +              iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
>>> +                               BIT(IS_S_RX_EVENT_SHIFT));
>>> +      }
>>
>> Why clearing one rx interrupt bit here after scheduling tasklet? Should all that
>> be done by tasklet? Also should just return after scheduling tasklet?

Regards,
Dhananjay