Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4bd8ce51-5874-0aa3-bc82-fec0cee9b8f1@arm.com>
Date:   Fri, 21 Apr 2023 15:58:18 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [RFC v1 3/4] swiotlb: Allow dynamic allocation of bounce buffers
Content-Language: en-GB
To:     =?UTF-8?B?UGV0ciBUZXNhxZnDrWs=?= <petr@tesarici.cz>,
        Christoph Hellwig <hch@lst.de>
Cc:     Petr Tesarik <petrtesarik@huaweicloud.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Borislav Petkov <bp@suse.de>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Randy Dunlap <rdunlap@infradead.org>,
        Damien Le Moal <damien.lemoal@opensource.wdc.com>,
        Kim Phillips <kim.phillips@amd.com>,
        "Steven Rostedt (Google)" <rostedt@goodmis.org>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:DMA MAPPING HELPERS" <iommu@lists.linux.dev>,
        Roberto Sassu <roberto.sassu@huawei.com>,
        Alexander Graf <graf@amazon.com>
References: <cover.1679309810.git.petr.tesarik.ext@huawei.com>
 <0334a54332ab75312c9de825548b616439dcc9f5.1679309810.git.petr.tesarik.ext@huawei.com>
 <20230328040724.GB25506@lst.de>
 <4268fa4e-4f0f-a2f6-a2a5-5b78ca4a073d@huaweicloud.com>
 <8cf7c515-9ce6-a2ed-0643-972aa3eba2fb@huaweicloud.com>
 <20230407055704.GD6803@lst.de> <20230407121555.4290a011@meshulam.tesarici.cz>
 <20230421150349.35966e0b@meshulam.tesarici.cz>
From:   Robin Murphy <robin.murphy@arm.com>
In-Reply-To: <20230421150349.35966e0b@meshulam.tesarici.cz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 2023-04-21 14:03, Petr Tesařík wrote:
> Hi Christoph!
> 
> I'd like to follow up on this sub-thread:
> 
> On Fri, 7 Apr 2023 12:15:55 +0200
> Petr Tesařík <petr@tesarici.cz> wroe:
> 
>> On Fri, 7 Apr 2023 07:57:04 +0200
>> Christoph Hellwig <hch@lst.de> wrote:
>> [...]
>>> (Btw, in case anyone is interested, we really need to get started
>>> on moving the dma fields out of struct device into a sub-struct
>>> only allocated for DMA capable busses)
>>
>> I like this idea. In fact, my WIP topic branch now moves the swiotlb
>> fields into a separate struct,
> 
> As you have noticed, I have removed that commit again in v2.
> 
> The reason is that I'm not sure about the intended goal. I have looked
> around for examples of moving fields out of struct device and found
> different approaches:
> 
> A. struct dev_msi_info
>     The MSI fields are merely grouped in a separate struct, which is
>     defined in device.h and embedded in struct device. I don't see much
>     benefit.
> 
> B. struct dev_pm_info
>     This struct is also embedded in struct device, but it is defined in
>     <linux/pm.h>, which is mentioned in MAINTAINERS. The benefit is that
>     further changes are reviewed by this maintainer. The downside is
>     that device.h includes pm.h.
> 
> C. struct dev_pin_info
>     This struct is merely declared in device.h and defined
>     pinctrl/devinfo.h (which is not included). Only a pointer to this
>     struct is stored in struct device. Of course, the pointer must be
>     initialized (and released) somehow.
> 
> Here my question: What did you want for DMA fields?
> 
> A. Only grouping those fields in their own struct?
> B. Or move the definition to another include file (cf. MAINTAINERS)?
> C. Or store a pointer in struct device?

dev->dma_parms is already this, and IIRC still has some very old 
comments somewhere about consolidating the other DMA-related fields in 
there.

> Since you mentioned "allocated", it sounds like you want to achieve C,
> but:
> 
> 1. Is it worth the extra dereference for every use?
> 2. How should the struct be allocated? Presumably not with kmalloc() in
>     device_initialize(), because I don't know how to determine if a
>     device is DMA capable this low in the call stack. So, should it be
>     allocated together with the containing structure? AFAICS this would
>     mean changing nearly all device drivers...

The bus code knows whether it's a DMA-capable bus or not, and as such 
should already be providing a .dma_configure method and/or performing 
some initialisation of DMA fields. Many of the ones that would need to 
are already providing dma_parms, in fact.

Thanks,
Robin.

> 
> As you can see, I need some more guidance from you before I can start
> working on this. ;-)
> 
> Petr T