Received-SPF: pass (google.com: domain of linux-kernel+bounces-71353-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <6b003271-cd83-4091-89c6-bb37da62afef@intel.com>
Date: Mon, 19 Feb 2024 13:53:36 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v3 1/7] dma: compile-out DMA sync op calls when
 not used
Content-Language: en-US
To: Robin Murphy <robin.murphy@arm.com>
CC: "David S. Miller" <davem@davemloft.net>, Eric Dumazet
	<edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, Christoph Hellwig <hch@lst.de>, Marek Szyprowski
	<m.szyprowski@samsung.com>, Joerg Roedel <joro@8bytes.org>, Will Deacon
	<will@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael
 J. Wysocki" <rafael@kernel.org>, Magnus Karlsson <magnus.karlsson@intel.com>,
	Maciej Fijalkowski <maciej.fijalkowski@intel.com>, Alexander Duyck
	<alexanderduyck@fb.com>, <bpf@vger.kernel.org>, <netdev@vger.kernel.org>,
	<iommu@lists.linux.dev>, <linux-kernel@vger.kernel.org>
References: <20240214162201.4168778-1-aleksander.lobakin@intel.com>
 <20240214162201.4168778-2-aleksander.lobakin@intel.com>
 <893ad3a4-ba24-43cf-8200-b8cd7742622d@arm.com>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <893ad3a4-ba24-43cf-8200-b8cd7742622d@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?T2J0MW9HSmRXWlc5WWVoaktSRUhING1RYUhmRTVLbjRhTkpqL1lYQUc4RVVF?=
 =?utf-8?B?Z2U5LzRac0lYRFFCNGhvTHJsM084ekhTZkJIRXJHaWF4eFRkdTVFS0FTM0F4?=
 =?utf-8?B?akR1OHBCVm44dGpXMk11bnVWMnVRQldWSlM4WGc4bFFYVGpzTktpWmRTZWFi?=
 =?utf-8?B?b2lYU3pJZmRnd0xCREJLMzAvSmN3K2RlSWNYU0RFNkRScEJZNEdrZDQvME1L?=
 =?utf-8?B?K3M0NkhGR0cyeVQvMC8rWlVEano3VTE0bHQxZWZ3RXhFWm93Z213ZW9NY0Q5?=
 =?utf-8?B?eWdtZlF4V1ExMEgzM3pnZ3Y5Z0l1dXpsdHowVnkrMmY0MUhscjdpMEo0TC9R?=
 =?utf-8?B?aU40MG9FME5aaUQyTFI3M1N0WnYxQnNpeWs2NzFsRG5wSklndXZUR0VLSnpz?=
 =?utf-8?B?QW01Ym1FbndzaTR5UisyekdmQlFoVS9zRFRxa1M2Q0hFQ0lKUWZ6Y1kzWXBa?=
 =?utf-8?B?a2dMOWxkYU9ZTUZnRlpNLzIwZWlIOUpRNThTZjBXMmZERzYzRFdMMy9ON3lt?=
 =?utf-8?B?NmY5STJtbzRqaThENjhQanhaL3ZyZFp1eXNBcUZIMGcxKzhJMXVvdmQ2emI5?=
 =?utf-8?B?V3VCSXFzdjg5RkcrRko1Y0ZSL0Uvd0taMGJCakNxM2NKU2huZ29QcE5TNnp2?=
 =?utf-8?B?bC9hRkZmOXBkbm00QXg1S2Q1Nnh4dTJLbGpQc3E0MERnY2ZDMzFzNHZLUDRv?=
 =?utf-8?B?aVJodnhmbjlxYWpUSFlFYjc0S1ZVN2V2NzRUNEVjREtzZGU3WWpQcW5yVCtt?=
 =?utf-8?B?T3VabjlIbDNZVWRPZWNPV2tSWkZmbnkyZCtJV0oyNzJJbWIvZzNwU2JUWk1K?=
 =?utf-8?B?YmNzc0t0bktMV0FGYnpsamtqS0s3MTRKdGpHL29tbWNTUXQ5Sk9ZVVpYVW96?=
 =?utf-8?B?K3o2UzZvdHB5RWF3YlZqOWhCc083ZkNkNSt0OUtvVVp5NTlSUUxDengyY040?=
 =?utf-8?B?MVkwcXhmUTZteVU4Mlg3aEhCQysxbjJmblFHbnFKSWttQTRpcjFab0JaQlU2?=
 =?utf-8?B?NEN2ckZoaVA5U1JWRVZtbDNEdm5kbTRieWEwZDMyVmNHNVluTHhQSlJSckdC?=
 =?utf-8?B?dU00V3MwNG4vNWFINlVMaEptM05MSmR5b0dOZEJEUTRpeWRuQjlsU0ovSzBu?=
 =?utf-8?B?NG1ibEowRmtsMGd6ZEt6cXRxTW5GcjNxbUpkOStrM0JadExRekR0SmQwb0Ir?=
 =?utf-8?B?Zi9mbzI1K1NGUVArbXlCQzUyNWdwZ2VuVE1xT3orZ2xCQWJ6aVhFV0Y0WnZQ?=
 =?utf-8?B?M2p4aGhOQ2RFTkVyeFBlM01yakhvemtKRE45djVPMUJCUjNtNzZVaWRjU1RM?=
 =?utf-8?B?eFZic25uWUFNUFlvcGsyMXhCR2pMRGxZNm01Rm9JbkFLbnMvNW91OUZ3SDR0?=
 =?utf-8?B?d25hSXBjMzVQaDFXV3VKRHgySU9ndXkxMTZqakRQaG92NjZKM0QxdVUrMUlQ?=
 =?utf-8?B?SDJoSzMyellVcisxdEdwZnVyeWwyaFpHOXRTZkxHSFZ4OUlaSjBVa0ZwSWNp?=
 =?utf-8?B?YnJlKzY0UGl0cU5WdDBGeUlNM0ZSVnlyT1FZSzVjZ2FidUdSTld3c0dneE5u?=
 =?utf-8?B?bVNVd2V0L0FGejhLbmx6ZmxGc0NFVmFmQzNRRjZmWEk5c1FhbjR6a3JzL2xV?=
 =?utf-8?B?REExMDB0bGVPRDBkZTYwUHM1bVp6eHpEdEQ4U1NVWG9VQTNZSXRuaEt2eU1m?=
 =?utf-8?B?QWJVcU1sWUc0dzRndTk0U1BRUHV4RjdtZDdPYW1kMmd5QUVSUkh1dkxNYk1I?=
 =?utf-8?B?ZDlqOWNSRXNPbTFBNTdUVndSOEdzNjd0YXRPVWVTeFlTbElpcFVBTFZJZ0My?=
 =?utf-8?B?N2REU0M1S2kzV3p3TXVuWFFycjM5OC82Qm9Pa0lHc05PVFJRbEtKbGdkcHNj?=
 =?utf-8?B?d2ZjaEl3VXJBTGlaMjg1dXR2d1pZMmt4TThSeXlGNDUwMDIvd1dLUkU3cTVa?=
 =?utf-8?B?VEVzRUFZT2w5cDNJS3BDTS9xTExqOHBmOG5wK2pzcjBmTmxKZXgvSHhCUmFa?=
 =?utf-8?B?dTBOYTZtdWJxWmtEMXBVZXdRc243WmNTT2RLbGF4dHRRbWx1a1RRV0xTZzBI?=
 =?utf-8?B?eFA0RFQ1RWVibUV1ems4NG5UZDd5RjNrY2V4VHRzSzIwZ0svL3VmYzhJYzdo?=
 =?utf-8?B?SGhrQkx6M0pyVFFTUUs1R2R1bzdoMTUxa3llczBmendMZktMSVFtREw0dCsy?=
 =?utf-8?B?R0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2ea1d434-bc1a-4c31-0689-08dc3149cccc
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Feb 2024 12:53:41.9338
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: VfgHYHXCr8urgYRz7uJLKE99SHcd93zSJl03gOVR/BK/tePAyUm1ejcesmSA0DU5sTippcCrUDm97isyfLp+TLeaOrfiQB6jb7CX/sfU/EE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB8166
X-OriginatorOrg: intel.com

From: Robin Murphy <robin.murphy@arm.com>
Date: Wed, 14 Feb 2024 17:20:50 +0000

> On 2024-02-14 4:21 pm, Alexander Lobakin wrote:

[...]

>> -static inline void dma_sync_single_for_cpu(struct device *dev,
>> dma_addr_t addr,
>> -        size_t size, enum dma_data_direction dir)
>> +static inline void __dma_sync_single_for_cpu(struct device *dev,
>> +        dma_addr_t addr, size_t size, enum dma_data_direction dir)
> 
> To me it would feel more logical to put all the wrappers inside the
> #ifdef CONFIG_HAS_DMA and not touch these stubs at all (what does it
> mean to skip an inline no-op?). Or in fact, if dma_skip_sync() is
> constant false for !HAS_DMA, then we could also just make the external
> function declarations unconditional and remove the stubs. Not a critical
> matter though, and I defer to whatever Christoph thinks is most
> maintainable.

It's done like that due to that I'm adding a runtime check in the second
patch. I don't feel like touching this twice makes sense.

[...]

>> @@ -348,18 +348,72 @@ static inline void dma_unmap_single_attrs(struct
>> device *dev, dma_addr_t addr,
>>       return dma_unmap_page_attrs(dev, addr, size, dir, attrs);
>>   }
>>   +static inline void __dma_sync_single_range_for_cpu(struct device *dev,
>> +        dma_addr_t addr, unsigned long offset, size_t size,
>> +        enum dma_data_direction dir)
>> +{
>> +    __dma_sync_single_for_cpu(dev, addr + offset, size, dir);
>> +}
>> +
>> +static inline void __dma_sync_single_range_for_device(struct device
>> *dev,
>> +        dma_addr_t addr, unsigned long offset, size_t size,
>> +        enum dma_data_direction dir)
>> +{
>> +    __dma_sync_single_for_device(dev, addr + offset, size, dir);
>> +}
> 
> There is no need to introduce these two.

I already replied to this in the previous thread. Some subsys may want
to check for the shortcut earlier to avoid call ladders of their own
functions. See patch 6 for example where I use this one.

> 
>> +
>> +static inline bool dma_skip_sync(const struct device *dev)
>> +{
>> +    return !IS_ENABLED(CONFIG_DMA_NEED_SYNC);
>> +}
>> +
>> +static inline bool dma_need_sync(struct device *dev, dma_addr_t
>> dma_addr)
>> +{
>> +    return !dma_skip_sync(dev) ? __dma_need_sync(dev, dma_addr) : false;
>> +}
> 
> That's a bit of a mind-bender... is it actually just
> 
>     return !dma_skip_sync(dev) && __dma_need_sync(dev, dma_addr);

Oh, indeed ._.

> 
> ?
> 
> (I do still think the negative flag makes it all a little harder to
> follow in general than a positive "device needs to consider syncs" flag
> would.)

I think it was in the original Eric's idea and I kept this.
I'm fine with inverting it.

[...]

> Thanks,
> Robin.

Thanks,
Olek