Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <f7441bb4-c2c9-4eee-9fed-ad8b28de4788@intel.com>
Date:   Fri, 13 Oct 2023 19:00:37 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()
Content-Language: en-US
To:     Dan Williams <dan.j.williams@intel.com>, <nvdimm@lists.linux.dev>,
        <linux-acpi@vger.kernel.org>
CC:     <rafael@kernel.org>, <vishal.l.verma@intel.com>, <lenb@kernel.org>,
        <dave.jiang@intel.com>, <ira.weiny@intel.com>,
        <linux-kernel@vger.kernel.org>,
        Andy Shevchenko <andy.shevchenko@gmail.com>
References: <20231013085722.3031537-1-michal.wilczynski@intel.com>
 <6529727e18964_f879294ea@dwillia2-mobl3.amr.corp.intel.com.notmuch>
From:   "Wilczynski, Michal" <michal.wilczynski@intel.com>
In-Reply-To: <6529727e18964_f879294ea@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N09PVFdvNXBsME9hSjJhM0JQRFBZUjByS1NrOU9wcThyWk9IQWhFdnMxNERK?=
 =?utf-8?B?enVUVGRTVXhzRXNlMVdPcmMrcC9JV3ROaVY1Y0F4V2JxaEpjRWsydStQcms3?=
 =?utf-8?B?MEhzZXczSDRwd1RrWXVaRms2UVNvZVBsQ1NTWll1dStoVy92ZjJtSldTOU5C?=
 =?utf-8?B?SUlIODdTOWVPaWczeXZOYWZhZ2xlYkp2V2VJdWlvekdlbnZMRUJyV0VNM0sy?=
 =?utf-8?B?Qnk0K1JadGxqVUZhQzJPUjVGSDlFbzRXcS9MbFlHc2k1M1kzMWtaNmQ1Rnpr?=
 =?utf-8?B?RzdQbjRkYW1KdFFMSVdjY0xCRG9nVUtBZWVQVGJHZXJxaVhVbE1uVHZZNnp0?=
 =?utf-8?B?RS8zMldrN0dJUWErY1pSNEZoNkxBb0Q4NzVjZ2hkaEVnS0VXZGRmVk9qSE9T?=
 =?utf-8?B?bHQ4cTVrMjRyQ3NIQXNGQXFmVFZxR3ArZ0NSK2hqRVFoMlRVZmZwZnpCQTJS?=
 =?utf-8?B?SzIzSktVcjNUeGdaVUdEMXg4MzZWNmVUWlhaMGRpNnRkTTlDMzBXN0hXcnpJ?=
 =?utf-8?B?RmxFdWpJQXhiUGRpeDU5VXA4WjRhWFUyd2ZvVDRsSDh4c0ZCbTVLbnhvTVJt?=
 =?utf-8?B?Qm1ZSFlXR1lrUi96Z1dNaFlvRWJWUTFncjE5Y0JqQ0JWWWZDTXc4SFZieHp0?=
 =?utf-8?B?TktHdm1FZkd2SndmOUdzWnZwNGF0emZzbk9BaERlVWFDQkcwNUdBcDBlYlRq?=
 =?utf-8?B?ZHB0WmNxdzNoRVQ3RlNmdlRzNnUxejkrRGxVZnpMaitlQXNhNXV6NUl3cEZZ?=
 =?utf-8?B?cmh5U0IvT2owS243OUJOREt5ZHROVGFWUHAzVjBOVGpWY2UxWDBqaFVvQ2sz?=
 =?utf-8?B?WHcxM0tNZXo0ZmNjM0FrdUFHc3RWK2VnUXJVNGt3bjBlNFhtT0djM0VZMnNO?=
 =?utf-8?B?ekRBV1dTTjhkdlJnZjd1RWFyTU9xOVlSOXJtWVd6Vy95aU1FQmgvOEFGZ1c5?=
 =?utf-8?B?TzU1QTJEQWs5bkRKSHRmUnNxZDRsOEYxQ2NnWmtzdy8vZDFrQzVQOUlzWmd6?=
 =?utf-8?B?U3B3V3pTMDErNXNGS2R2eml5NHAvYmF6VFN5VHN0bmtyNVpXTHA3cGZxMXRS?=
 =?utf-8?B?aDAvUVYyZWJRbC9FVXJnVTlidFN3dkxyTUYwUk9GZGNEd2Z6d1BUTEkxOGFP?=
 =?utf-8?B?Wk05T0cvcURkbjJVZ2lkdXEvN2FHcXBlVEV0cC9ocWhOZ1lrTFNEYU9pL2dO?=
 =?utf-8?B?c2RGOWgyS3B4UWxGeGhUV29DR0NBTktBaUN5UTljbSswTTRTc0VqWWNXUGlE?=
 =?utf-8?B?R21TWDYrMndKUWw0N0VvUEJQUnVXcDVHUmg5QUFoWEh3T2JTbTZYUXJyckVr?=
 =?utf-8?B?aUdoZThpWjQxalNVUjBEaWI3OWNqMlBSVW1zR0pxMXVNemZLWnA2ODdhY1RJ?=
 =?utf-8?B?QjhUMkhtMnRBOUJkSWZSMnBoa2dwNWthdWJuK0NiMWg4THgrMFdWaVI0ZFhD?=
 =?utf-8?B?U0tPZ2NaYVFxZ25oalU4ZGk4OXppM3dtUnlFcnZHUXlQMEJVWEZGUWVzRGN4?=
 =?utf-8?B?V0JGQVpkMk9vTVU2WkJ6aVNTMXR6djc0V21wSmJrUkROcnEvdUNVL3ZoSEph?=
 =?utf-8?B?eU5VUTlIZXJhQ0Q4clVNYnNMTzBDcVJHM2tmZWZ5dERSMW5EZ0VWcUhLa2Vt?=
 =?utf-8?B?d1ZMUDRESXFxbjVWNUxjYWJzeXQyV3AxbmExRVNmV21zaks4L1FlYXRIUzU5?=
 =?utf-8?B?MkJVeEFJVjNCWEZkd2xWZUtzaHNBMlFTMXpnOGQ1Y2lnU293bVRORkpKZWto?=
 =?utf-8?B?RkFGZ0Y2NHlGdkh1RFg1KzJoTEd3NmE5cGhLb1BGRUlncHQwVUpJYXZ6MndR?=
 =?utf-8?B?UlJEZkR2cURSbnVFUjZRQ0dwaFIvek1FVFZncGRrWTgyS1JHN0xZd2ZqYVQ0?=
 =?utf-8?B?VlNTY3pTTDdsR21weXYvMzZMempQVDR2U25sYVNLc3oxY3FCYmhkVFZRV3Zj?=
 =?utf-8?B?UTd4WUJoajFra0dLN1VxUjl3dGNDNjRnWVBkSFpPRFlJVE5FMHQ0MDJDem13?=
 =?utf-8?B?TzU1OGx1QTdHQjJLZDlXYmI2cTlLVk11TmY3eUMwS2JhSjhqWE5nNnU3TnRR?=
 =?utf-8?B?OXRLUFBrN2E2VzBQVktXMWUxWjEvVmdpM25vMEpnVmF1RzNFRnMzRzhTMWlX?=
 =?utf-8?B?SVhmZ2xqWDFpbSs4K0pDWEhtcUhwZzBvc0lkU0tEVWs1cGFpaWc1cjkrYWZn?=
 =?utf-8?B?N1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f584e2b6-7e7d-4334-d535-08dbcc0df096
X-MS-Exchange-CrossTenant-AuthSource: CO6PR11MB5603.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Oct 2023 17:00:44.8430
 (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: Tg39R9NBy4Nk6rO+HP4mJeeLRac4PJiyLZbPp2QV84ly5d2n0+E9QMxx/0pF3+DLij7BH9sbVvbO7bZRj+QSVlMoz/dqMZoowVmP4BLJwo0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7365
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Fri, 13 Oct 2023 10:01:02 -0700 (PDT)


On 10/13/2023 6:38 PM, Dan Williams wrote:
> Michal Wilczynski wrote:
>> devm_*() family of functions purpose is managing memory attached to a
>> device. So in general it should only be used for allocations that should
>> last for the whole lifecycle of the device. 
> No, this assertion is not accurate, if it were strictly true then
> devm_kfree() should be deleted. This patch is only a cleanup to switch
> the automatic cleanup pattern from devm to the new cleanup.h helpers.

The memory in question is only used locally in a function, so there is no reason
to use devm_*() family of functions. I think devm_kfree() is more for special
cases where the memory is meant to be used for the whole lifecycle of device,
but some special case occurs and it's not and it needs to be freed.

This is an incorrect API usage. Would you propose to change all memory
allocations currently being done to devm_*() family simply because devm_kfree()
exists ? Why introduce extra overhead if you don't have to ?

>
> I am all for modernizing code over time, but patches that make
> assertions of "memory leaks" and "incorrect API usage" in code that has
> been untouched for almost a decade demand more scrutiny than what
> transpired here.

I understand that it's not necessarily a big problem, and the code works
perfectly, I can change the phrasing if you don't like it, but still the
cleanup.h helpers don't really care that much what functions they call
to allocate/free. They are meant to care about the scope - like constructor
destructor in C++ - you can call anything there.

So this commit changes 2 things:

- change family of function to allocate from
   devm_kcalloc() to kcalloc()
- use scope based mechanism to call those functions


Thanks a lot for your review !
Michał