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: <d2124ddc-7e62-96af-62c5-2c44f181fe66@redhat.com>
Date:   Wed, 16 Nov 2022 16:39:52 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: [PATCH v2 5/7] platform/x86: ideapad-laptop: Expose camera_power
 only if supported
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
To:     =?UTF-8?Q?Eray_Or=c3=a7unus?= <erayorcunus@gmail.com>,
        platform-driver-x86@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        ike.pan@canonical.com, jikos@kernel.org,
        benjamin.tissoires@redhat.com, dmitry.torokhov@gmail.com,
        mgross@linux.intel.com, pobrn@protonmail.com
References: <20221029120311.11152-1-erayorcunus@gmail.com>
 <20221029120311.11152-6-erayorcunus@gmail.com>
 <dde0d934-45be-8b12-bdf6-0c3de7321731@redhat.com>
In-Reply-To: <dde0d934-45be-8b12-bdf6-0c3de7321731@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Eray,

On 11/15/22 21:43, Hans de Goede wrote:
> Hi Eray,
> 
> On 10/29/22 14:03, Eray Orçunus wrote:
>> IdeaPads dropped support for VPCCMD_W_CAMERA somewhere between 2014-2016,
>> none of the IdeaPads produced after that I tested supports it. Fortunately
>> I found a way to check it; if the DSDT has camera device(s) defined, it
>> shouldn't have working VPCCMD_W_CAMERA, thus camera_power shouldn't be
>> exposed to sysfs. To accomplish this, walk the ACPI namespace in
>> ideapad_check_features and check the devices starting with "CAM".
>> Tested on 520-15IKB and Legion Y520, which successfully didn't expose
>> the camera_power attribute.
>>
>> Link: https://www.spinics.net/lists/platform-driver-x86/msg26147.html
>> Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
>> ---
>>  drivers/platform/x86/ideapad-laptop.c | 52 ++++++++++++++++++++++++++-
>>  1 file changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>> index f3d4f2beda07..e8c088e7a53d 100644
>> --- a/drivers/platform/x86/ideapad-laptop.c
>> +++ b/drivers/platform/x86/ideapad-laptop.c
>> @@ -149,6 +149,7 @@ struct ideapad_private {
>>  		bool fn_lock              : 1;
>>  		bool hw_rfkill_switch     : 1;
>>  		bool kbd_bl               : 1;
>> +		bool cam_ctrl_via_ec      : 1;
>>  		bool touchpad_ctrl_via_ec : 1;
>>  		bool usb_charging         : 1;
>>  	} features;
>> @@ -163,6 +164,24 @@ static bool no_bt_rfkill;
>>  module_param(no_bt_rfkill, bool, 0444);
>>  MODULE_PARM_DESC(no_bt_rfkill, "No rfkill for bluetooth.");
>>  
>> +static acpi_status acpi_find_device_callback(acpi_handle handle, u32 level,
>> +					     void *context, void **return_value)
>> +{
>> +	struct acpi_buffer ret_buf;
>> +	char buffer[8];
>> +
>> +	ret_buf.length = sizeof(buffer);
>> +	ret_buf.pointer = buffer;
>> +
>> +	if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_SINGLE_NAME, &ret_buf)))
>> +		if (strstarts(ret_buf.pointer, context)) {
>> +			*return_value = handle;
>> +			return AE_CTRL_TERMINATE;
>> +		}
>> +
>> +	return AE_OK;
>> +}
>> +
>>  /*
>>   * ACPI Helpers
>>   */
>> @@ -675,7 +694,7 @@ static umode_t ideapad_is_visible(struct kobject *kobj,
>>  	bool supported = true;
>>  
>>  	if (attr == &dev_attr_camera_power.attr)
>> -		supported = test_bit(CFG_CAP_CAM_BIT, &priv->cfg);
>> +		supported = priv->features.cam_ctrl_via_ec;
>>  	else if (attr == &dev_attr_conservation_mode.attr)
>>  		supported = priv->features.conservation_mode;
>>  	else if (attr == &dev_attr_fan_mode.attr)
>> @@ -1527,6 +1546,37 @@ static void ideapad_check_features(struct ideapad_private *priv)
>>  
>>  	priv->features.hw_rfkill_switch = dmi_check_system(hw_rfkill_list);
>>  
>> +	/*
>> +	 * Some IdeaPads have camera switch via EC (mostly older ones),
>> +	 * some don't. Fortunately we know that if DSDT contains device
>> +	 * object for the camera, camera isn't switchable via EC.
>> +	 * So, let's walk the namespace and try to find CAM* object.
>> +	 * If we can't find it, set cam_ctrl_via_ec to true.
>> +	 */
>> +
>> +	priv->features.cam_ctrl_via_ec = false;
> 
> There is no need to explicitly set this to false since the entire
> struct is allocated with kzalloc() and a bunch of other features
> flags are also not explicitly set to false. Please drop this line.
> 
>> +
>> +	if (test_bit(CFG_CAP_CAM_BIT, &priv->cfg)) {
>> +		acpi_handle temp_handle = NULL;
>> +		acpi_handle pci_handle;
>> +		acpi_status status;
>> +
>> +		status = acpi_get_handle(handle, "^^^", &pci_handle);
>> +		if (ACPI_SUCCESS(status)) {
>> +			status = acpi_walk_namespace(ACPI_TYPE_DEVICE, pci_handle,
>> +						     ACPI_UINT32_MAX,
>> +						     acpi_find_device_callback,
>> +						     NULL, "CAM",
>> +						     &temp_handle);
> 
> Why not just use acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ... ?
> 
> The PCI root is usually pretty much the only object under the root anyways
> and this way you can avoid the acpi_get_handle() call + its error handling,
> so using ACPI_ROOT_OBJECT would lead to a nice cleanup.

Note when you send out a new version of this patch + patch 6/7,
please base it on top of my current review-hans branch since
a bunch of other ideapad-laptop changes have landed there.

Regards,

Hans