linux-kernel - Re: [PATCH 1/2] hwmon: (k8temp) update to use new hwmon registration API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <325bcd3e-7795-1b34-587f-38364dd477f4@roeck-us.net>
Date:   Sat, 20 Jul 2019 14:12:08 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Robert Karszniewicz <avoidr@...email.cc>,
        Rudolf Marek <r.marek@...embler.cz>,
        Jean Delvare <jdelvare@...e.com>
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] hwmon: (k8temp) update to use new hwmon registration
 API

On 7/20/19 2:05 PM, Robert Karszniewicz wrote:
> Hello Guenter.
> 
> Thank you for your feedback! I am working on a version 2.
> 
> On Sat Jul 20, 2019 at 7:08 AM Guenter Roeck wrote:
>>> +static umode_t
>>> +k8temp_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>>> +		  u32 attr, int channel)
>>> +{
>>> +	if (type != hwmon_temp)
>>> +		return 0;
>>
>> Not really needed since only temperature sensors are registered in the first place.
>>
>>> +
>>> +	if (attr != hwmon_temp_input)
>>> +		return 0;
>>> +
>>
>> The idea here is to only create sensors if they actually exist, so I would expect
>> something like the following here.
> 
> I realised that in the probe() function I deleted all the code which
> conditionally creates the sysfs files. Is that how it's supposed to be
> done and that's exactly what is_visible() is for, or is the proper way
> to still create the sysfs files conditionally, too?
> 

Yes, that is what the function is for. sysfs files are created in the core,
based on the result from the call to the is_visible function. If the function
returns 0, the respective sysfs file won't be created.

Thanks,
Guenter

>> 	struct k8temp_data *data = drvdata;
>>
>> 	if (attr != hwmon_temp_input)
>> 		return 0;
>>
>> 	if ((channel & 1) && !(data->sensorsp & SEL_PLACE))
>> 		return 0;
>>
>> 	if ((channel & 2) && !(data->sensorsp & SEL_CORE))
>> 		return 0;
>>
>>> +	return 0444;
>>> +}
> 
>>> +	if (data->swap_core_select)
>>> +		core = core ? 0 : 1;
>>
>> 		core = 1 - core;
>>
>> would accomplish the same without conditional.
> 
> How do you like
> 	core ^= 1;
> ?
> 
>>>    static int k8temp_probe(struct pci_dev *pdev,
>>>    				  const struct pci_device_id *id)
>>>    {
>>> -	int err;
>>>    	u8 scfg;
>>>    	u32 temp;
>>>    	u8 model, stepping;
>>>    	struct k8temp_data *data;
>>> +	struct device *hwmon_dev;
>>>    
>>>    	data = devm_kzalloc(&pdev->dev, sizeof(struct k8temp_data), GFP_KERNEL);
>>>    	if (!data)
>>> @@ -233,84 +274,23 @@ static int k8temp_probe(struct pci_dev *pdev,
>>>    
>>>    	data->name = "k8temp";
>>>    	mutex_init(&data->update_lock);
>>> -	pci_set_drvdata(pdev, data);
>>> -
>>> -	/* Register sysfs hooks */
>>> -	err = device_create_file(&pdev->dev,
>>> -			   &sensor_dev_attr_temp1_input.dev_attr);
>>> -	if (err)
>>> -		goto exit_remove;
>>> -
>>> -	/* sensor can be changed and reports something */
>>> -	if (data->sensorsp & SEL_PLACE) {
>>> -		err = device_create_file(&pdev->dev,
>>> -				   &sensor_dev_attr_temp2_input.dev_attr);
>>> -		if (err)
>>> -			goto exit_remove;
>>> -	}
>>> -
>>> -	/* core can be changed and reports something */
>>> -	if (data->sensorsp & SEL_CORE) {
>>> -		err = device_create_file(&pdev->dev,
>>> -				   &sensor_dev_attr_temp3_input.dev_attr);
>>> -		if (err)
>>> -			goto exit_remove;
>>> -		if (data->sensorsp & SEL_PLACE) {
>>> -			err = device_create_file(&pdev->dev,
>>> -					   &sensor_dev_attr_temp4_input.
>>> -					   dev_attr);
>>> -			if (err)
>>> -				goto exit_remove;
>>> -		}
>>> -	}
>