From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751203AbdBDPfG (ORCPT ); Sat, 4 Feb 2017 10:35:06 -0500 Received: from mail-qk0-f194.google.com ([209.85.220.194]:35669 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750876AbdBDPfE (ORCPT ); Sat, 4 Feb 2017 10:35:04 -0500 MIME-Version: 1.0 In-Reply-To: References: <20170126153013.12753-1-jprvita@endlessm.com> <20170126153013.12753-8-jprvita@endlessm.com> From: Andy Shevchenko Date: Sat, 4 Feb 2017 17:35:03 +0200 Message-ID: Subject: Re: [PATCH 7/8] asus-wireless: Export ids list To: =?UTF-8?Q?Jo=C3=A3o_Paulo_Rechi_Vita?= Cc: Corentin Chary , Darren Hart , Platform Driver , acpi4asus-user , "linux-kernel@vger.kernel.org" , linux@endlessm.com, =?UTF-8?Q?Jo=C3=A3o_Paulo_Rechi_Vita?= Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v14FZHVO030114 On Wed, Feb 1, 2017 at 2:23 PM, João Paulo Rechi Vita wrote: > On 27 January 2017 at 10:36, Andy Shevchenko wrote: >> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita >> wrote: Fill commit message, btw. >>> Signed-off-by: João Paulo Rechi Vita >>> -static const struct acpi_device_id device_ids[] = { >>> - {"ATK4001", 0}, >>> - {"ATK4002", 0}, >>> - {"", 0}, >>> -}; >>> -MODULE_DEVICE_TABLE(acpi, device_ids); >>> +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids); >>> >> >> No, Don't do this. >> > > Why? Table is a property of certain driver. You make it visible to parts that non need it. Moreover, you may here the list itself non-explicit, which reduces readability and understandability worse. If you would like to maintain a list of devices in two (semi-)independent modules, it would be not good looking in any case, either you make a hard dependency (if they already are it's okay to just export a function which helps you to find an ID in the list), or copy it in both modules. I need to check this as well. >>> static u64 asus_wireless_method(acpi_handle handle, const char *method, >>> int param) >>> @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev) >>> adev->driver_data = data; >>> >>> hid = acpi_device_hid(adev); >>> - for (i = 0; strcmp(device_ids[i].id, ""); i++) { >> >> This is wrong. >> >>> - if (!strcmp(device_ids[i].id, hid)) { >>> + for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) { >> >> This is too. >> >> Potential infinite loop. >> >> On top of that seems you just introduced this by previous patches and >> changing here. >> Often it means you need to reconsider how you actually split the >> series on logical pieces. >> > > Can you please elaborate a bit more? The original code relies on "" in the first parameter which basically can be NULL. This fragile. But this is part subject to change in a sequential patch. > All this change does is to change > the name of the variable being iterated in the loop. As you said, the > loop was introduced in a previous series, and you didn't spot any > problems there. If I didn't spot it doesn't mean there is no issues, right? ;) > I don't think it makes sense to also rename the > variable in that other series, since I'm only renaming it because I'm > moving it to a header so it can be used by asus-wmi.c as well. The main problem with the above is introduction something that you are changing soon later. -- With Best Regards, Andy Shevchenko