linux-kernel - Toshiba Bluetooth enabler (v2) (was: Re: [PATCH] Toshiba Bluetooth enabler (rfkill))

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4B1E85A9.3030005@gmail.com>
Date:	Tue, 08 Dec 2009 17:58:17 +0100
From:	Jes Sorensen <jes.sorensen@...il.com>
To:	Alan Jenkins <sourcejedi.lkml@...glemail.com>
CC:	linux-acpi@...r.kernel.org,
	linux-kernel <linux-kernel@...r.kernel.org>, lenb@...nel.org,
	Matthew Garrett <mjg59@...f.ucam.org>
Subject: Toshiba Bluetooth enabler (v2) (was: Re: [PATCH] Toshiba Bluetooth
 enabler (rfkill))

On 12/08/2009 11:05 AM, Alan Jenkins wrote:
> Cool.  This must be the shortest ACPI driver :-).

Hi Alan,

Here's an updated version that addresses most of your comments.

> Try to send patches inline if possible, it makes them easier to review.

Not possible with Thunderbug, sorry.

>> +static int toshiba_bt_rfkill_add(struct acpi_device *device);
>
> I think it's simpler to avoid the need for forward declarations, e.g.
> by putting the driver structure after the ops functions.  I suppose
> it's a matter of taste though.

I am going to keep it as is. I find it cleaner and it is more consistent
with how other drivers are written IMHO.

> Please also set
> 	.owner = THIS_MODULE,

Good point, thanks!

>> +static int toshiba_bt_resume(struct acpi_device *device)
>> +{
>> +	return toshiba_bluetooth_enable(device->handle);
>> +}
>
> Good to see you're handling resume, but what happens if the rfkill
> switch is _not_ set to on?  It looks like resume will return an error,
> which will produce a warning message in the kernel log.  I don't think
> we want that.

Fixed, this version only calls the enabler if the switch is at ON at
resume time.

>> +	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
>> +				&bt_present);
>
> I think this would benefit from an explanation.

Comments added.

>> +	result = acpi_bus_register_driver(&toshiba_bt_rfkill_driver);
>> +	if (result<  0) {
>> +		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
>> +				  "Error registering driver\n"));
>> +		return -ENODEV;
>> +	}
>
> I would suggest you return result, instead of ignoring it and always
> returning ENODEV.

I agree, added.

>
>> +	depends on RFKILL
>> +	depends on BT
>
> But you doesn't use either of these subsystems :-). The BT one
> definitely seems bogus; please drop it.

It seemed kinda silly to me to enable this driver on kernels with no
BT subsystem, but it's not a biggie so I pulled it.

Please find attached v2 of the patch.

Cheers,
Jes


View attachment "0002-toshiba-bluetooth-v2.patch" of type "text/plain" (6412 bytes)