linux-kernel - Re: [RESEND PATCH v2 4/8] mfd: stm32-timers: add support for dmas

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: <805c6c6c-4984-1436-56a7-a44d23a06429@st.com>
Date:   Thu, 29 Mar 2018 17:29:45 +0200
From:   Fabrice Gasnier <fabrice.gasnier@...com>
To:     Lee Jones <lee.jones@...aro.org>
CC:     <thierry.reding@...il.com>, <alexandre.torgue@...com>,
        <benjamin.gaignard@...aro.org>, <robh+dt@...nel.org>,
        <mark.rutland@....com>, <linux@...linux.org.uk>,
        <mcoquelin.stm32@...il.com>, <benjamin.gaignard@...com>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-pwm@...r.kernel.org>
Subject: Re: [RESEND PATCH v2 4/8] mfd: stm32-timers: add support for dmas

On 03/29/2018 04:31 PM, Lee Jones wrote:
> On Thu, 29 Mar 2018, Fabrice Gasnier wrote:
> 
>> On 03/29/2018 02:59 PM, Lee Jones wrote:
>>> On Wed, 28 Mar 2018, Fabrice Gasnier wrote:
>>>
>>>> On 03/28/2018 05:22 PM, Lee Jones wrote:
>>>>> On Wed, 14 Feb 2018, Fabrice Gasnier wrote:
>>>>>
>>>>>> STM32 Timers can support up to 7 DMA requests:
>>>>>> - 4 channels, update, compare and trigger.
>>>>>> Optionally request part, or all DMAs from stm32-timers MFD core.
>>>>>>
>>>>>> Also add routine to implement burst reads using DMA from timer registers.
>>>>>> This is exported. So, it can be used by child drivers, PWM capture
>>>>>> for instance (but not limited to).
>>>>>>
>>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...com>
>>>>>> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@...aro.org>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> - Abstract DMA handling from child driver: move it to MFD core
>>>>>> - Add comments on optional dma support
>>>>>> ---
>>>>>>  drivers/mfd/stm32-timers.c       | 215 ++++++++++++++++++++++++++++++++++++++-
>>>>>>  include/linux/mfd/stm32-timers.h |  27 +++++
>>>>>>  2 files changed, 238 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
>>>>>> index a6675a4..2cdad2c 100644
>>>>>> --- a/drivers/mfd/stm32-timers.c
>>>>>> +++ b/drivers/mfd/stm32-timers.c
>>>
>>> [...]
>>>
>>>>>> +	struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
>>>>>> +	struct stm32_timers ddata;
>>>>>
>>>>> This looks odd to me.  Why can't you expand the current ddata
>>>>> structure?  Wouldn't it be better to create a stm32_timers_dma
>>>>> structure to place all this information in (except *dev, that should
>>>>> live in the ddata struct), then place a reference in the existing
>>>>> stm32_timers struct?
>>>>
>>>> Maybe I miss-understand you here, from what we discussed in V1:
>>>> https://lkml.org/lkml/2018/1/23/574
>>>>> ... "passing in the physical address of the parent MFD into
>>>>> a child device doesn't quite sit right with me"
>>>> I introduced this private struct in MFD parent, and completely hide it
>>>> from the child.
>>>>
>>>> So, do you suggest to add struct definition here ? But make it part of
>>>> struct stm32_timers *ddata?
>>>>
>>>> And only put declaration in include/linux/mfd/stm32-timers.h:
>>>> + struct stm32_timers_dma;
>>>>
>>>> struct stm32_timers {
>>>> 	struct clk *clk;
>>>> 	struct regmap *regmap;
>>>> 	u32 max_arr;
>>>> +	struct stm32_timers_dma;
>>>> };
>>>
>>> Yes, that's the basic idea.
>>>
>>>> I can probably spare the *dev then... use dev->parent in child driver.
>>>
>>> What would you use dev->parent for?
>>
>> Hi Lee,
>>
>> This is to follow your sugestion to use *dev instead of *ddata when
>> calling stm32_timers_dma_burst_read(), the idea is to use it on child side:
>> stm32_timers_dma_burst_read(dev->parent,...) from pwm driver.
>> Then there is no need to keep *dev inside ddata struct.
> 
> I'm wondering if it would be neater to us the child's *dev, then do
> the ->parent deference in the parent MFD (with a comment to say what
> you're doing of course).
> 

There's already dev.parent dereference in child drivers, for same
purpose: dev_get_drvdata(pdev->dev.parent). So, I guess same can be done
here ?

Thanks for you review,
I'll update all this and send a v3.

Best regards,
Fabrice

>>> [...]
>>>
>>>>>> +static int stm32_timers_remove(struct platform_device *pdev)
>>>>>> +{
>>>>>> +	struct stm32_timers *ddata = platform_get_drvdata(pdev);
>>>>>> +	struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
>>>>>> +
>>>>>> +	of_platform_depopulate(&pdev->dev);
>>>>>
>>>>> Why can't you continue using devm_*?
>>>>
>>>> I can use devm_of_platform_depopulate() here if you prefer, and keep
>>>> devm_of_platform_populate() in probe.
>>>
>>> The point of devm_* is that you don't have to call depopulate.
>>>
>>> It happens automatically once this driver is unbound.
>>
>> Ok, so to clarify, keeping devm_ here may be a bit racy:
>> of_platform_depopulate will happen after dma has been released (there is
>> no devm_ variant to release dma).
>> Only way to prevent race condition here, is to enforce
>> of_platform_depopulate() is called before dma release (e.g. in reverse
>> order compared to probe).
>>
>> Do you wish I add a comment about it ?
> 
> Best thing to do then is keep the non-devm variant and provide a
> comment as to why is it not possible to use devm_*.
>