Re: [RFC PATCH] input: Add disable sysfs entry for every input device
On Tuesday 03 January 2017 12:21:21 Bastien Nocera wrote: > On Mon, 2017-01-02 at 18:09 +0100, Pali Rohár wrote: > > On Monday 02 January 2017 16:27:05 Bastien Nocera wrote: > > > On Sun, 2016-12-25 at 11:04 +0100, Pali Rohár wrote: > > > > This patch allows user to disable events from any input device so > > > > events > > > > would not be delivered to userspace. > > > > > > > > Currently there is no way to disable particular input device by > > > > kernel. > > > > User for different reasons would need it for integrated PS/2 > > > > keyboard or > > > > touchpad in notebook or touchscreen on mobile device to prevent > > > > sending > > > > events. E.g. mobile phone in pocket or broken integrated PS/2 > > > > keyboard. > > > > > > > > This is just a RFC patch, not tested yet. Original post about > > > > motivation > > > > about this patch is there: https://lkml.org/lkml/2014/11/29/92 > > > > > > Having implemented something of that ilk in user-space (we > > > automatically disable touch devices when the associated screen is > > > turned off/suspended), I think this might need more thought. > > > > How to implement such thing in userspace? I think you cannot do that > > without rewriting every one userspace application which uses input. > > > > > What happens when a device is opened and the device disabled > > through > > > sysfs, are the users revoked? > > > > Applications will not receive events. Same as if input device does > > not > > generates events. > > > > > Does this put the device in suspend in the same way that closing > > the > > > device's last user does? > > > > Current code not (this is just RFC prototype), but it should be > > possible > > to implement. > > > > > Is this not better implemented in user-space at the session level, > > > where it knows about which output corresponds to which input > > device? > > > > How to do that without rewriting existing applications? > > > > > Is this useful enough to disable misbehaving devices on hardware, > > so > > > that the device is not effective on boot? > > > > In case integrated device is absolutely unusable and generates > > always > > random events, it does not solve problem at boot time. > > > > But more real case is laptop with closed LID press buttons and here > > it > > is useful. > > There's usually a display manager in between the application and the > input device. But that is not always truth. In some cases there are applications which opens input device directly. > Whether it's X.org, or a Wayland compositor. Even David's > https://github.com/dvdhrm/kmscon could help for console applications. That kmscon needs KMS, right? So it would not work on hardware which do not have KMS drivers. -- Pali Rohár pali.ro...@gmail.com
Re: [RFC PATCH] input: Add disable sysfs entry for every input device
On Tuesday 03 January 2017 12:21:21 Bastien Nocera wrote: > On Mon, 2017-01-02 at 18:09 +0100, Pali Rohár wrote: > > On Monday 02 January 2017 16:27:05 Bastien Nocera wrote: > > > On Sun, 2016-12-25 at 11:04 +0100, Pali Rohár wrote: > > > > This patch allows user to disable events from any input device so > > > > events > > > > would not be delivered to userspace. > > > > > > > > Currently there is no way to disable particular input device by > > > > kernel. > > > > User for different reasons would need it for integrated PS/2 > > > > keyboard or > > > > touchpad in notebook or touchscreen on mobile device to prevent > > > > sending > > > > events. E.g. mobile phone in pocket or broken integrated PS/2 > > > > keyboard. > > > > > > > > This is just a RFC patch, not tested yet. Original post about > > > > motivation > > > > about this patch is there: https://lkml.org/lkml/2014/11/29/92 > > > > > > Having implemented something of that ilk in user-space (we > > > automatically disable touch devices when the associated screen is > > > turned off/suspended), I think this might need more thought. > > > > How to implement such thing in userspace? I think you cannot do that > > without rewriting every one userspace application which uses input. > > > > > What happens when a device is opened and the device disabled > > through > > > sysfs, are the users revoked? > > > > Applications will not receive events. Same as if input device does > > not > > generates events. > > > > > Does this put the device in suspend in the same way that closing > > the > > > device's last user does? > > > > Current code not (this is just RFC prototype), but it should be > > possible > > to implement. > > > > > Is this not better implemented in user-space at the session level, > > > where it knows about which output corresponds to which input > > device? > > > > How to do that without rewriting existing applications? > > > > > Is this useful enough to disable misbehaving devices on hardware, > > so > > > that the device is not effective on boot? > > > > In case integrated device is absolutely unusable and generates > > always > > random events, it does not solve problem at boot time. > > > > But more real case is laptop with closed LID press buttons and here > > it > > is useful. > > There's usually a display manager in between the application and the > input device. But that is not always truth. In some cases there are applications which opens input device directly. > Whether it's X.org, or a Wayland compositor. Even David's > https://github.com/dvdhrm/kmscon could help for console applications. That kmscon needs KMS, right? So it would not work on hardware which do not have KMS drivers. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900
On Friday 10 November 2017 00:38:22 Pali Rohár wrote: > This patch series fix processing MAC address for wl1251 chip found in Nokia > N900. > > Changes since v1: > * Added Acked-by for Pavel Machek > * Fixed grammar > * Magic numbers for NVS offsets are replaced by defines > * Check for validity of mac address NVS data is moved into function > * Changed order of patches as Pavel requested > > Pali Rohár (6): > wl1251: Update wl->nvs_len after wl->nvs is valid > wl1251: Generate random MAC address only if driver does not have > valid > wl1251: Parse and use MAC address from supplied NVS data > wl1251: Set generated MAC address back to NVS data > firmware: Add request_firmware_prefer_user() function > wl1251: Use request_firmware_prefer_user() for loading NVS > calibration data > > drivers/base/firmware_class.c | 45 +- > drivers/net/wireless/ti/wl1251/Kconfig |1 + > drivers/net/wireless/ti/wl1251/main.c | 104 > ++-- > include/linux/firmware.h |9 +++ > 4 files changed, 138 insertions(+), 21 deletions(-) Hi! Are there any comments for first 4 patches? If not, could they be accepted and merged? -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900
On Friday 10 November 2017 00:38:22 Pali Rohár wrote: > This patch series fix processing MAC address for wl1251 chip found in Nokia > N900. > > Changes since v1: > * Added Acked-by for Pavel Machek > * Fixed grammar > * Magic numbers for NVS offsets are replaced by defines > * Check for validity of mac address NVS data is moved into function > * Changed order of patches as Pavel requested > > Pali Rohár (6): > wl1251: Update wl->nvs_len after wl->nvs is valid > wl1251: Generate random MAC address only if driver does not have > valid > wl1251: Parse and use MAC address from supplied NVS data > wl1251: Set generated MAC address back to NVS data > firmware: Add request_firmware_prefer_user() function > wl1251: Use request_firmware_prefer_user() for loading NVS > calibration data > > drivers/base/firmware_class.c | 45 +- > drivers/net/wireless/ti/wl1251/Kconfig |1 + > drivers/net/wireless/ti/wl1251/main.c | 104 > ++-- > include/linux/firmware.h |9 +++ > 4 files changed, 138 insertions(+), 21 deletions(-) Hi! Are there any comments for first 4 patches? If not, could they be accepted and merged? -- Pali Rohár pali.ro...@gmail.com
Race-free unlinking of directory entries
Hi! Linux kernel currently does not provide any race-free way for calling unlink() syscall on file entry which points to opened file descriptor. On the other hand Linux kernel already provides race-free way for creating file entry by linkat() syscall with AT_EMPTY_PATH or AT_SYMLINK_FOLLOW flags. unlinkat() does not. There was already discussion about unlink issue in bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=93441 Because file descriptor describes inode number which can be stored in more directories as hard links, there is a proposed funlinkat() syscall with following API: int funlinkat(int fd, int dirfd, const char *pathname, int flags); It should atomically check if file descriptor fd and pathname (according to dirfd) are same, and if then just unlinkat(dirfd, pathname, flags). If are not same, throw error. What userspace application basically needs: Open file, test it stat (or probably content) and based on test result decide if file needs to be removed or not. Or delete a file behind a file descriptor opened with O_PATH. Both cases are currently not possible without introducing race condition between open/stat and unlink. Between those two calls, some other process can exchange files. -- Pali Rohár pali.ro...@gmail.com
Race-free unlinking of directory entries
Hi! Linux kernel currently does not provide any race-free way for calling unlink() syscall on file entry which points to opened file descriptor. On the other hand Linux kernel already provides race-free way for creating file entry by linkat() syscall with AT_EMPTY_PATH or AT_SYMLINK_FOLLOW flags. unlinkat() does not. There was already discussion about unlink issue in bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=93441 Because file descriptor describes inode number which can be stored in more directories as hard links, there is a proposed funlinkat() syscall with following API: int funlinkat(int fd, int dirfd, const char *pathname, int flags); It should atomically check if file descriptor fd and pathname (according to dirfd) are same, and if then just unlinkat(dirfd, pathname, flags). If are not same, throw error. What userspace application basically needs: Open file, test it stat (or probably content) and based on test result decide if file needs to be removed or not. Or delete a file behind a file descriptor opened with O_PATH. Both cases are currently not possible without introducing race condition between open/stat and unlink. Between those two calls, some other process can exchange files. -- Pali Rohár pali.ro...@gmail.com
Re: Linux & FAT32 label
On Thursday 09 November 2017 22:21:31 Pali Rohár wrote: > So from all tests and discussion I would propose new unification: > > 1. Read label only from the root directory. If label in root directory >is missing then disk would be treated as without label. Label from >boot sector would not be read. > >--> Reason: Windows XP and mlabel ignores what is written in boot >sector. Windows XP even do not update boot sector, so label >stored in boot sector is incorrect after any change done by >Windows XP. > >This logic is used by all tested MS-DOS and Windows versions, >plus also by mtools on Linux. > > 2. Write label to to both location, boot sector and root directory. > >--> Reason: MS-DOS 6.22, MS-DOS 7.10, Windows 98 and also mtools on >Linux do this. This is also what is written in FAT specification. > >It also provides backward compatibility with old dosfslabel >versions which read label only from boot sector. > > 2. Process 'NO NAME' label in root directory as 'NO NAME' name. Not >as empty label. > >--> Reason: 'NO NAME' is regular entry in root directory and both >Windows XP and mlabel handle it in this way. > > 3. Process 'NO NAME' label in boot directory as empty label. Not as >label with name 'NO NAME'. > >--> Reason: On Windows XP when formatting empty disk and label is not >specified then 'NO NAME' is stored to boot sector. > >Also in FAT specification is written that empty label is stored >as 'NO NAME'. > > With this change we would get compatibility with MS-DOS, Windows (both > DOS-based and NT-based) and also with Linux mtools, modulo problems DOS > code page. > > There are just two negatives: > > 1) Labels set by old dosfslabel versions (which stored them only to boot >sector) would not be visible. But they are already not visible on >MS-DOS or Windows machines, and also via mlabel (from mtools). > > 2) Behavior of blkid and fatlabel would be changed as both tools have >different as proposed above, and based on tests they also differ each > from other. > > Andreas, Karel, what do you think about it? Andreas, any comments? It is OK? More then month passed... and I would like to move forward. -- Pali Rohár pali.ro...@gmail.com
Re: Linux & FAT32 label
On Thursday 09 November 2017 22:21:31 Pali Rohár wrote: > So from all tests and discussion I would propose new unification: > > 1. Read label only from the root directory. If label in root directory >is missing then disk would be treated as without label. Label from >boot sector would not be read. > >--> Reason: Windows XP and mlabel ignores what is written in boot >sector. Windows XP even do not update boot sector, so label >stored in boot sector is incorrect after any change done by >Windows XP. > >This logic is used by all tested MS-DOS and Windows versions, >plus also by mtools on Linux. > > 2. Write label to to both location, boot sector and root directory. > >--> Reason: MS-DOS 6.22, MS-DOS 7.10, Windows 98 and also mtools on >Linux do this. This is also what is written in FAT specification. > >It also provides backward compatibility with old dosfslabel >versions which read label only from boot sector. > > 2. Process 'NO NAME' label in root directory as 'NO NAME' name. Not >as empty label. > >--> Reason: 'NO NAME' is regular entry in root directory and both >Windows XP and mlabel handle it in this way. > > 3. Process 'NO NAME' label in boot directory as empty label. Not as >label with name 'NO NAME'. > >--> Reason: On Windows XP when formatting empty disk and label is not >specified then 'NO NAME' is stored to boot sector. > >Also in FAT specification is written that empty label is stored >as 'NO NAME'. > > With this change we would get compatibility with MS-DOS, Windows (both > DOS-based and NT-based) and also with Linux mtools, modulo problems DOS > code page. > > There are just two negatives: > > 1) Labels set by old dosfslabel versions (which stored them only to boot >sector) would not be visible. But they are already not visible on >MS-DOS or Windows machines, and also via mlabel (from mtools). > > 2) Behavior of blkid and fatlabel would be changed as both tools have >different as proposed above, and based on tests they also differ each > from other. > > Andreas, Karel, what do you think about it? Andreas, any comments? It is OK? More then month passed... and I would like to move forward. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH] platform/x86: dell-rbtn: Block comments use * on subsequent lines
On Wednesday 13 December 2017 12:41:03 Dhaval Shah wrote: > I have given this SPDX ID based on the current license info in the driver. > Yeah. I give the authority to author pali.ro...@gmail.com to decide the SPDX > text. Hi! I think that license of this module is clear and does not need more comments. Part "either version 2 of the License, or (at your option) any later version." expresses what it exactly mean and it is standard license also used by other projects. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH] platform/x86: dell-rbtn: Block comments use * on subsequent lines
On Wednesday 13 December 2017 12:41:03 Dhaval Shah wrote: > I have given this SPDX ID based on the current license info in the driver. > Yeah. I give the authority to author pali.ro...@gmail.com to decide the SPDX > text. Hi! I think that license of this module is clear and does not need more comments. Part "either version 2 of the License, or (at your option) any later version." expresses what it exactly mean and it is standard license also used by other projects. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH] platform/x86: dell-smo8800: Possible unnecessary 'out of memory' message
On Wednesday 13 December 2017 13:53:54 Dhaval Shah wrote:
> Removed Possible unnecessary 'out of memory' message checkpatch warnings.
> Issue found by checkpatch.
>
> Signed-off-by: Dhaval Shah <dhaval.s...@softnautics.com>
> ---
> drivers/platform/x86/dell-smo8800.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-smo8800.c
> b/drivers/platform/x86/dell-smo8800.c
> index 1d87237bc731..9590d5e7c5ca 100644
> --- a/drivers/platform/x86/dell-smo8800.c
> +++ b/drivers/platform/x86/dell-smo8800.c
> @@ -150,10 +150,8 @@ static int smo8800_add(struct acpi_device *device)
> struct smo8800_device *smo8800;
>
> smo8800 = devm_kzalloc(>dev, sizeof(*smo8800), GFP_KERNEL);
> - if (!smo8800) {
> - dev_err(>dev, "failed to allocate device data\n");
Hi! Any particular reason for removing error message?
> + if (!smo8800)
> return -ENOMEM;
> - }
>
> smo8800->dev = >dev;
> smo8800->miscdev.minor = MISC_DYNAMIC_MINOR;
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH] platform/x86: dell-smo8800: Possible unnecessary 'out of memory' message
On Wednesday 13 December 2017 13:53:54 Dhaval Shah wrote:
> Removed Possible unnecessary 'out of memory' message checkpatch warnings.
> Issue found by checkpatch.
>
> Signed-off-by: Dhaval Shah
> ---
> drivers/platform/x86/dell-smo8800.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-smo8800.c
> b/drivers/platform/x86/dell-smo8800.c
> index 1d87237bc731..9590d5e7c5ca 100644
> --- a/drivers/platform/x86/dell-smo8800.c
> +++ b/drivers/platform/x86/dell-smo8800.c
> @@ -150,10 +150,8 @@ static int smo8800_add(struct acpi_device *device)
> struct smo8800_device *smo8800;
>
> smo8800 = devm_kzalloc(>dev, sizeof(*smo8800), GFP_KERNEL);
> - if (!smo8800) {
> - dev_err(>dev, "failed to allocate device data\n");
Hi! Any particular reason for removing error message?
> + if (!smo8800)
> return -ENOMEM;
> - }
>
> smo8800->dev = >dev;
> smo8800->miscdev.minor = MISC_DYNAMIC_MINOR;
--
Pali Rohár
pali.ro...@gmail.com
Re: i8k_smm_func() takes enormous of time to execute
On Monday 04 December 2017 13:08:18 Oleksandr Natalenko wrote: > Hi. > > 04.12.2017 11:35, Pali Rohár wrote: > > On Friday 24 November 2017 13:28:59 Oleksandr Natalenko wrote: > > > On pátek 24. listopadu 2017 12:25:43 CET Pali Rohár wrote: > > > > On Friday 24 November 2017 12:17:30 Oleksandr Natalenko wrote: > > > > > > There are two patches waiting to be tested in > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=195751 > > > > > > > > > > Tested and attached a couple of patches on top of those to the BZ. If > > > > > disabling fan control is the only approach here, I can confirm that > > > > > this > > > > > works. > > > > > > > > Hi! Please figure out which SMM call (they are identified by eax and ebx > > > > registers) cause this freeze. So we would now what needs to be > > > > blacklisted. > > > > > > Here it goes: > > > > > > [ 7.191081] dell_smm_hwmon: smm(0x10a3 0x) = 0x002e (took > > > 1837 usecs) > > > [ 7.194007] dell_smm_hwmon: smm(0x00a3 0x0001) = 0x (took > > > 151 usecs) > > > [ 7.198239] dell_smm_hwmon: smm(0x00a3 0x) = 0x0002 (took > > > 1411 usecs) > > > [ 7.199095] dell_smm_hwmon: smm(0x02a3 0x0001) = 0x (took > > > 71 usecs) > > > [ 7.700493] dell_smm_hwmon: smm(0x02a3 0x) = 0x157c (took > > > 488912 usecs) > > > [ 7.701277] dell_smm_hwmon: smm(0x0025 0X) = 0x (took > > > 71 usecs) > > > > > > 0x02a3 is I8K_SMM_GET_SPEED. > > > > Ok, so it is really a good idea to disable fan control completely on > > your machine. > > This still doesn't explain why things work in non-Linux environment… No, they does not work in non-Linux environment too. You can call same smm request also on Windows and it freeze computer in same way. Just windows do not have such smm driver out of box. To reproduce this problem you need to write either own kernel driver in assembler (like driver for linux) or write that smm code in assembler for classic (userspace) application and find out how to call that smm code via WinAPI (probably you would need Administrator rights and study lot of WinAPI documentation). > But as a workaround, okay, let it be. Basically once you issue this 0x02a3 call your computer freeze (more precisely stay in SMM mode -- it is x86 processor mode) and such operation is fully independent of what is running on your x86 cpu (Linux, Windows, DOS, some RTOS system, ...) > > Your last patch in bugzilla looks ok, you add my Reviewed-by: Pali > > Rohár <pali.ro...@gmail.com> > > Could you please advice on how to proceed further? I can submit all 3 > patches (incl. yours two), to a ML. Now it is up to hwmon maintainers (Jean Delvare & Guenter Roeck) to pick up these patches. I think nothing more is needed from your side. (And if yes, Jean would write.) -- Pali Rohár pali.ro...@gmail.com
Re: i8k_smm_func() takes enormous of time to execute
On Monday 04 December 2017 13:08:18 Oleksandr Natalenko wrote: > Hi. > > 04.12.2017 11:35, Pali Rohár wrote: > > On Friday 24 November 2017 13:28:59 Oleksandr Natalenko wrote: > > > On pátek 24. listopadu 2017 12:25:43 CET Pali Rohár wrote: > > > > On Friday 24 November 2017 12:17:30 Oleksandr Natalenko wrote: > > > > > > There are two patches waiting to be tested in > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=195751 > > > > > > > > > > Tested and attached a couple of patches on top of those to the BZ. If > > > > > disabling fan control is the only approach here, I can confirm that > > > > > this > > > > > works. > > > > > > > > Hi! Please figure out which SMM call (they are identified by eax and ebx > > > > registers) cause this freeze. So we would now what needs to be > > > > blacklisted. > > > > > > Here it goes: > > > > > > [ 7.191081] dell_smm_hwmon: smm(0x10a3 0x) = 0x002e (took > > > 1837 usecs) > > > [ 7.194007] dell_smm_hwmon: smm(0x00a3 0x0001) = 0x (took > > > 151 usecs) > > > [ 7.198239] dell_smm_hwmon: smm(0x00a3 0x) = 0x0002 (took > > > 1411 usecs) > > > [ 7.199095] dell_smm_hwmon: smm(0x02a3 0x0001) = 0x (took > > > 71 usecs) > > > [ 7.700493] dell_smm_hwmon: smm(0x02a3 0x) = 0x157c (took > > > 488912 usecs) > > > [ 7.701277] dell_smm_hwmon: smm(0x0025 0X) = 0x (took > > > 71 usecs) > > > > > > 0x02a3 is I8K_SMM_GET_SPEED. > > > > Ok, so it is really a good idea to disable fan control completely on > > your machine. > > This still doesn't explain why things work in non-Linux environment… No, they does not work in non-Linux environment too. You can call same smm request also on Windows and it freeze computer in same way. Just windows do not have such smm driver out of box. To reproduce this problem you need to write either own kernel driver in assembler (like driver for linux) or write that smm code in assembler for classic (userspace) application and find out how to call that smm code via WinAPI (probably you would need Administrator rights and study lot of WinAPI documentation). > But as a workaround, okay, let it be. Basically once you issue this 0x02a3 call your computer freeze (more precisely stay in SMM mode -- it is x86 processor mode) and such operation is fully independent of what is running on your x86 cpu (Linux, Windows, DOS, some RTOS system, ...) > > Your last patch in bugzilla looks ok, you add my Reviewed-by: Pali > > Rohár > > Could you please advice on how to proceed further? I can submit all 3 > patches (incl. yours two), to a ML. Now it is up to hwmon maintainers (Jean Delvare & Guenter Roeck) to pick up these patches. I think nothing more is needed from your side. (And if yes, Jean would write.) -- Pali Rohár pali.ro...@gmail.com
Re: i8k_smm_func() takes enormous of time to execute
On Friday 24 November 2017 13:28:59 Oleksandr Natalenko wrote: > On pátek 24. listopadu 2017 12:25:43 CET Pali Rohár wrote: > > On Friday 24 November 2017 12:17:30 Oleksandr Natalenko wrote: > > > > There are two patches waiting to be tested in > > > > https://bugzilla.kernel.org/show_bug.cgi?id=195751 > > > > > > Tested and attached a couple of patches on top of those to the BZ. If > > > disabling fan control is the only approach here, I can confirm that this > > > works. > > > > Hi! Please figure out which SMM call (they are identified by eax and ebx > > registers) cause this freeze. So we would now what needs to be > > blacklisted. > > Here it goes: > > [ 7.191081] dell_smm_hwmon: smm(0x10a3 0x) = 0x002e (took 1837 usecs) > [ 7.194007] dell_smm_hwmon: smm(0x00a3 0x0001) = 0x (took 151 usecs) > [ 7.198239] dell_smm_hwmon: smm(0x00a3 0x) = 0x0002 (took 1411 usecs) > [ 7.199095] dell_smm_hwmon: smm(0x02a3 0x0001) = 0x (took 71 usecs) > [ 7.700493] dell_smm_hwmon: smm(0x02a3 0x) = 0x157c (took 488912 usecs) > [ 7.701277] dell_smm_hwmon: smm(0x0025 0X) = 0x (took 71 usecs) > > 0x02a3 is I8K_SMM_GET_SPEED. Ok, so it is really a good idea to disable fan control completely on your machine. Your last patch in bugzilla looks ok, you add my Reviewed-by: Pali Rohár <pali.ro...@gmail.com> -- Pali Rohár pali.ro...@gmail.com
Re: i8k_smm_func() takes enormous of time to execute
On Friday 24 November 2017 13:28:59 Oleksandr Natalenko wrote: > On pátek 24. listopadu 2017 12:25:43 CET Pali Rohár wrote: > > On Friday 24 November 2017 12:17:30 Oleksandr Natalenko wrote: > > > > There are two patches waiting to be tested in > > > > https://bugzilla.kernel.org/show_bug.cgi?id=195751 > > > > > > Tested and attached a couple of patches on top of those to the BZ. If > > > disabling fan control is the only approach here, I can confirm that this > > > works. > > > > Hi! Please figure out which SMM call (they are identified by eax and ebx > > registers) cause this freeze. So we would now what needs to be > > blacklisted. > > Here it goes: > > [ 7.191081] dell_smm_hwmon: smm(0x10a3 0x) = 0x002e (took 1837 usecs) > [ 7.194007] dell_smm_hwmon: smm(0x00a3 0x0001) = 0x (took 151 usecs) > [ 7.198239] dell_smm_hwmon: smm(0x00a3 0x) = 0x0002 (took 1411 usecs) > [ 7.199095] dell_smm_hwmon: smm(0x02a3 0x0001) = 0x (took 71 usecs) > [ 7.700493] dell_smm_hwmon: smm(0x02a3 0x) = 0x157c (took 488912 usecs) > [ 7.701277] dell_smm_hwmon: smm(0x0025 0X) = 0x (took 71 usecs) > > 0x02a3 is I8K_SMM_GET_SPEED. Ok, so it is really a good idea to disable fan control completely on your machine. Your last patch in bugzilla looks ok, you add my Reviewed-by: Pali Rohár -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH] Support TrackStick of Thinkpad L570
On Monday 04 December 2017 09:40:04 Masaki Ota wrote:
> Hi, Pali,
>
> It does not work in my test result.
Hm.. that is strange, we have dangling pointers in struct alps_data?
Otherwise I have no idea why does not work.
> BTW, other some functions also use both of "struct psmouse" and "struct
> alps_data" argument.
It does not make sense to pass one structure (via pointers) two times.
And if this "pattern" is already used in code and reason is that one
pointer "does not work" because it is dangling, then it is really wrong.
I know it is irrelevant to your patch, but this problem with dangling
pointer should be fixed, e.g. in next patches (not in this one).
Problems (with memory allocation/pointers) should not be camouflaged.
Memory corruption in kernel can lead to fatal problems.
> I just followed it.
Blindly following bad code is a bad idea. When we see something like
this, we should at least stop and ask question "why is this code pattern
used?".
> Best Regards,
> Masaki Ota
> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Monday, December 04, 2017 6:12 PM
> To: 太田 真喜 Masaki Ota <masaki@jp.alps.com>
> Cc: Masaki Ota <012ne...@gmail.com>; dmitry.torok...@gmail.com;
> benjamin.tissoi...@redhat.com; aaron...@canonical.com; j...@ristioja.ee;
> linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
>
> On Monday 04 December 2017 04:48:43 Masaki Ota wrote:
> > Hi, Pali,
> >
> > I don't get your point.
> > Please modify the code if you have an idea.
>
> See below
>
> > Best Regards,
> > Masaki Ota
> > -Original Message-
> > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > Sent: Saturday, December 02, 2017 6:08 AM
> > To: Masaki Ota <012ne...@gmail.com>
> > Cc: dmitry.torok...@gmail.com; benjamin.tissoi...@redhat.com;
> > aaron...@canonical.com; j...@ristioja.ee; 太田 真喜 Masaki Ota
> > <masaki@jp.alps.com>; linux-in...@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
> >
> > On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> > > From: Masaki Ota <masaki@jp.alps.com>
> > > - The issue is that Thinkpad L570 TrackStick does not work. Because the
> > > main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2
> > > interface Firmware setting of TrackStick. The detail is that TrackStick
> > > otp bit is disabled.
> > > - Add the code that checks 0xD7 address value. This value is device
> > > number information, so we can identify the device by checking this value.
> > > - If we check 0xD7 value, we need to enable Command mode and after check
> > > the value we need to disable Command mode, then we have to enable the
> > > device(0xF4 command).
> > > - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable
> > > ALPS_DUALPOINT flag.
> > >
> > > Signed-off-by: Masaki Ota <masaki@jp.alps.com>
> > > ---
> > > drivers/input/mouse/alps.c | 24 +---
> > > 1 file changed, 21 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > > index 850b00e3ad8e..6f092bdd9fc5 100644
> > > --- a/drivers/input/mouse/alps.c
> > > +++ b/drivers/input/mouse/alps.c
> > > @@ -2541,13 +2541,31 @@ static int
> > > alps_update_btn_info_ss4_v2(unsigned char otp[][4], }
> > >
> > > static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> > > -struct alps_data *priv)
> > > +struct alps_data *priv,
> > > + struct psmouse *psmouse)
> > > {
>
> You can access psmouse from the priv structure as:
>
> struct psmouse *psmouse = priv->psmouse;
>
> Therefore you do not have to extend function parameters with psmouse pointer
> as that is already present int alps_data.
>
> struct alps_data is defined as:
>
> struct alps_data {
> struct psmouse *psmouse;
> ...
> }
>
> > > bool is_dual = false;
> > > + int reg_val = 0;
> > > + struct ps2dev *ps2dev = >ps2dev;
> > >
> > > - if (IS_SS4PLUS_DEV(priv->dev_id))
> > > + if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > > is_dual = (otp[0][0] >> 4) & 0x01;
> > >
> > > +
Re: [PATCH] Support TrackStick of Thinkpad L570
On Monday 04 December 2017 09:40:04 Masaki Ota wrote:
> Hi, Pali,
>
> It does not work in my test result.
Hm.. that is strange, we have dangling pointers in struct alps_data?
Otherwise I have no idea why does not work.
> BTW, other some functions also use both of "struct psmouse" and "struct
> alps_data" argument.
It does not make sense to pass one structure (via pointers) two times.
And if this "pattern" is already used in code and reason is that one
pointer "does not work" because it is dangling, then it is really wrong.
I know it is irrelevant to your patch, but this problem with dangling
pointer should be fixed, e.g. in next patches (not in this one).
Problems (with memory allocation/pointers) should not be camouflaged.
Memory corruption in kernel can lead to fatal problems.
> I just followed it.
Blindly following bad code is a bad idea. When we see something like
this, we should at least stop and ask question "why is this code pattern
used?".
> Best Regards,
> Masaki Ota
> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Monday, December 04, 2017 6:12 PM
> To: 太田 真喜 Masaki Ota
> Cc: Masaki Ota <012ne...@gmail.com>; dmitry.torok...@gmail.com;
> benjamin.tissoi...@redhat.com; aaron...@canonical.com; j...@ristioja.ee;
> linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
>
> On Monday 04 December 2017 04:48:43 Masaki Ota wrote:
> > Hi, Pali,
> >
> > I don't get your point.
> > Please modify the code if you have an idea.
>
> See below
>
> > Best Regards,
> > Masaki Ota
> > -Original Message-
> > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > Sent: Saturday, December 02, 2017 6:08 AM
> > To: Masaki Ota <012ne...@gmail.com>
> > Cc: dmitry.torok...@gmail.com; benjamin.tissoi...@redhat.com;
> > aaron...@canonical.com; j...@ristioja.ee; 太田 真喜 Masaki Ota
> > ; linux-in...@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
> >
> > On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> > > From: Masaki Ota
> > > - The issue is that Thinkpad L570 TrackStick does not work. Because the
> > > main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2
> > > interface Firmware setting of TrackStick. The detail is that TrackStick
> > > otp bit is disabled.
> > > - Add the code that checks 0xD7 address value. This value is device
> > > number information, so we can identify the device by checking this value.
> > > - If we check 0xD7 value, we need to enable Command mode and after check
> > > the value we need to disable Command mode, then we have to enable the
> > > device(0xF4 command).
> > > - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable
> > > ALPS_DUALPOINT flag.
> > >
> > > Signed-off-by: Masaki Ota
> > > ---
> > > drivers/input/mouse/alps.c | 24 +---
> > > 1 file changed, 21 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > > index 850b00e3ad8e..6f092bdd9fc5 100644
> > > --- a/drivers/input/mouse/alps.c
> > > +++ b/drivers/input/mouse/alps.c
> > > @@ -2541,13 +2541,31 @@ static int
> > > alps_update_btn_info_ss4_v2(unsigned char otp[][4], }
> > >
> > > static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> > > -struct alps_data *priv)
> > > +struct alps_data *priv,
> > > + struct psmouse *psmouse)
> > > {
>
> You can access psmouse from the priv structure as:
>
> struct psmouse *psmouse = priv->psmouse;
>
> Therefore you do not have to extend function parameters with psmouse pointer
> as that is already present int alps_data.
>
> struct alps_data is defined as:
>
> struct alps_data {
> struct psmouse *psmouse;
> ...
> }
>
> > > bool is_dual = false;
> > > + int reg_val = 0;
> > > + struct ps2dev *ps2dev = >ps2dev;
> > >
> > > - if (IS_SS4PLUS_DEV(priv->dev_id))
> > > + if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > > is_dual = (otp[0][0] >> 4) & 0x01;
> > >
> > > + if (!is_dual) {
> > > + /* For support TrackStick of Thinkpad L/E series */
> > > +
Re: [PATCH] Support TrackStick of Thinkpad L570
On Monday 04 December 2017 04:48:43 Masaki Ota wrote:
> Hi, Pali,
>
> I don't get your point.
> Please modify the code if you have an idea.
See below
> Best Regards,
> Masaki Ota
> -Original Message-----
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Saturday, December 02, 2017 6:08 AM
> To: Masaki Ota <012ne...@gmail.com>
> Cc: dmitry.torok...@gmail.com; benjamin.tissoi...@redhat.com;
> aaron...@canonical.com; j...@ristioja.ee; 太田 真喜 Masaki Ota
> <masaki@jp.alps.com>; linux-in...@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
>
> On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> > From: Masaki Ota <masaki@jp.alps.com>
> > - The issue is that Thinkpad L570 TrackStick does not work. Because the
> > main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2
> > interface Firmware setting of TrackStick. The detail is that TrackStick otp
> > bit is disabled.
> > - Add the code that checks 0xD7 address value. This value is device number
> > information, so we can identify the device by checking this value.
> > - If we check 0xD7 value, we need to enable Command mode and after check
> > the value we need to disable Command mode, then we have to enable the
> > device(0xF4 command).
> > - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable
> > ALPS_DUALPOINT flag.
> >
> > Signed-off-by: Masaki Ota <masaki@jp.alps.com>
> > ---
> > drivers/input/mouse/alps.c | 24 +---
> > 1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > index 850b00e3ad8e..6f092bdd9fc5 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -2541,13 +2541,31 @@ static int
> > alps_update_btn_info_ss4_v2(unsigned char otp[][4], }
> >
> > static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> > - struct alps_data *priv)
> > + struct alps_data *priv,
> > + struct psmouse *psmouse)
> > {
You can access psmouse from the priv structure as:
struct psmouse *psmouse = priv->psmouse;
Therefore you do not have to extend function parameters with psmouse
pointer as that is already present int alps_data.
struct alps_data is defined as:
struct alps_data {
struct psmouse *psmouse;
...
}
> > bool is_dual = false;
> > + int reg_val = 0;
> > + struct ps2dev *ps2dev = >ps2dev;
> >
> > - if (IS_SS4PLUS_DEV(priv->dev_id))
> > + if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > is_dual = (otp[0][0] >> 4) & 0x01;
> >
> > + if (!is_dual) {
> > + /* For support TrackStick of Thinkpad L/E series */
> > + if (alps_exit_command_mode(psmouse) == 0 &&
> > + alps_enter_command_mode(psmouse) == 0) {
> > + reg_val = alps_command_mode_read_reg(psmouse,
> > + 0xD7);
> > + }
> > + alps_exit_command_mode(psmouse);
> > + ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> > +
> > + if (reg_val == 0x0C || reg_val == 0x1D)
> > + is_dual = true;
> > + }
> > + }
> > +
> > if (is_dual)
> > priv->flags |= ALPS_DUALPOINT |
> > ALPS_DUALPOINT_WITH_PRESSURE;
> > @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct
> > psmouse *psmouse,
> >
> > alps_update_btn_info_ss4_v2(otp, priv);
> >
> > - alps_update_dual_info_ss4_v2(otp, priv);
> > + alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>
> Now looking at this change... Is there reason why you are passing psmouse
> parameter there? Because struct alps_data contains psmouse member.
>
> >
> > return 0;
> > }
>
> --
> Pali Rohár
> pali.ro...@gmail.com
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH] Support TrackStick of Thinkpad L570
On Monday 04 December 2017 04:48:43 Masaki Ota wrote:
> Hi, Pali,
>
> I don't get your point.
> Please modify the code if you have an idea.
See below
> Best Regards,
> Masaki Ota
> -Original Message-----
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Saturday, December 02, 2017 6:08 AM
> To: Masaki Ota <012ne...@gmail.com>
> Cc: dmitry.torok...@gmail.com; benjamin.tissoi...@redhat.com;
> aaron...@canonical.com; j...@ristioja.ee; 太田 真喜 Masaki Ota
> ; linux-in...@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
>
> On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> > From: Masaki Ota
> > - The issue is that Thinkpad L570 TrackStick does not work. Because the
> > main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2
> > interface Firmware setting of TrackStick. The detail is that TrackStick otp
> > bit is disabled.
> > - Add the code that checks 0xD7 address value. This value is device number
> > information, so we can identify the device by checking this value.
> > - If we check 0xD7 value, we need to enable Command mode and after check
> > the value we need to disable Command mode, then we have to enable the
> > device(0xF4 command).
> > - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable
> > ALPS_DUALPOINT flag.
> >
> > Signed-off-by: Masaki Ota
> > ---
> > drivers/input/mouse/alps.c | 24 +---
> > 1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > index 850b00e3ad8e..6f092bdd9fc5 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -2541,13 +2541,31 @@ static int
> > alps_update_btn_info_ss4_v2(unsigned char otp[][4], }
> >
> > static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> > - struct alps_data *priv)
> > + struct alps_data *priv,
> > + struct psmouse *psmouse)
> > {
You can access psmouse from the priv structure as:
struct psmouse *psmouse = priv->psmouse;
Therefore you do not have to extend function parameters with psmouse
pointer as that is already present int alps_data.
struct alps_data is defined as:
struct alps_data {
struct psmouse *psmouse;
...
}
> > bool is_dual = false;
> > + int reg_val = 0;
> > + struct ps2dev *ps2dev = >ps2dev;
> >
> > - if (IS_SS4PLUS_DEV(priv->dev_id))
> > + if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > is_dual = (otp[0][0] >> 4) & 0x01;
> >
> > + if (!is_dual) {
> > + /* For support TrackStick of Thinkpad L/E series */
> > + if (alps_exit_command_mode(psmouse) == 0 &&
> > + alps_enter_command_mode(psmouse) == 0) {
> > + reg_val = alps_command_mode_read_reg(psmouse,
> > + 0xD7);
> > + }
> > + alps_exit_command_mode(psmouse);
> > + ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> > +
> > + if (reg_val == 0x0C || reg_val == 0x1D)
> > + is_dual = true;
> > + }
> > + }
> > +
> > if (is_dual)
> > priv->flags |= ALPS_DUALPOINT |
> > ALPS_DUALPOINT_WITH_PRESSURE;
> > @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct
> > psmouse *psmouse,
> >
> > alps_update_btn_info_ss4_v2(otp, priv);
> >
> > - alps_update_dual_info_ss4_v2(otp, priv);
> > + alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>
> Now looking at this change... Is there reason why you are passing psmouse
> parameter there? Because struct alps_data contains psmouse member.
>
> >
> > return 0;
> > }
>
> --
> Pali Rohár
> pali.ro...@gmail.com
--
Pali Rohár
pali.ro...@gmail.com
Re: linux-next on Nokia N900: hang on boot
On Saturday 02 December 2017 11:14:56 Pavel Machek wrote: > Hi! > > next from 2017-12-01 was tested. > > MMC is detected, but then I see wtl4030_bci: .. battery temperature > out of range, and machine halts. This is just before userspace would > be started. > > 4.15-rc1 seems to work ok on the box. > > Any ideas? twl4030_bci is not available on Nokia N900, so we should not try to read its registers. -- Pali Rohár pali.ro...@gmail.com
Re: linux-next on Nokia N900: hang on boot
On Saturday 02 December 2017 11:14:56 Pavel Machek wrote: > Hi! > > next from 2017-12-01 was tested. > > MMC is detected, but then I see wtl4030_bci: .. battery temperature > out of range, and machine halts. This is just before userspace would > be started. > > 4.15-rc1 seems to work ok on the box. > > Any ideas? twl4030_bci is not available on Nokia N900, so we should not try to read its registers. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH] Support TrackStick of Thinkpad L570
On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> From: Masaki Ota <masaki@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main
> interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface
> Firmware setting of TrackStick. The detail is that TrackStick otp bit is
> disabled.
> - Add the code that checks 0xD7 address value. This value is device number
> information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the
> value we need to disable Command mode, then we have to enable the device(0xF4
> command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable
> ALPS_DUALPOINT flag.
>
> Signed-off-by: Masaki Ota <masaki@jp.alps.com>
> ---
> drivers/input/mouse/alps.c | 24 +---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..6f092bdd9fc5 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,31 @@ static int alps_update_btn_info_ss4_v2(unsigned char
> otp[][4],
> }
>
> static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -struct alps_data *priv)
> +struct alps_data *priv,
> + struct psmouse *psmouse)
> {
> bool is_dual = false;
> + int reg_val = 0;
> + struct ps2dev *ps2dev = >ps2dev;
>
> - if (IS_SS4PLUS_DEV(priv->dev_id))
> + if (IS_SS4PLUS_DEV(priv->dev_id)) {
> is_dual = (otp[0][0] >> 4) & 0x01;
>
> + if (!is_dual) {
> + /* For support TrackStick of Thinkpad L/E series */
> + if (alps_exit_command_mode(psmouse) == 0 &&
> + alps_enter_command_mode(psmouse) == 0) {
> + reg_val = alps_command_mode_read_reg(psmouse,
> + 0xD7);
> + }
> + alps_exit_command_mode(psmouse);
> + ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> + if (reg_val == 0x0C || reg_val == 0x1D)
> + is_dual = true;
> + }
> + }
> +
> if (is_dual)
> priv->flags |= ALPS_DUALPOINT |
> ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse
> *psmouse,
>
> alps_update_btn_info_ss4_v2(otp, priv);
>
> - alps_update_dual_info_ss4_v2(otp, priv);
> + alps_update_dual_info_ss4_v2(otp, priv, psmouse);
Now looking at this change... Is there reason why you are passing
psmouse parameter there? Because struct alps_data contains psmouse
member.
>
> return 0;
> }
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH] Support TrackStick of Thinkpad L570
On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> From: Masaki Ota
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main
> interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface
> Firmware setting of TrackStick. The detail is that TrackStick otp bit is
> disabled.
> - Add the code that checks 0xD7 address value. This value is device number
> information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the
> value we need to disable Command mode, then we have to enable the device(0xF4
> command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable
> ALPS_DUALPOINT flag.
>
> Signed-off-by: Masaki Ota
> ---
> drivers/input/mouse/alps.c | 24 +---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..6f092bdd9fc5 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,31 @@ static int alps_update_btn_info_ss4_v2(unsigned char
> otp[][4],
> }
>
> static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -struct alps_data *priv)
> +struct alps_data *priv,
> + struct psmouse *psmouse)
> {
> bool is_dual = false;
> + int reg_val = 0;
> + struct ps2dev *ps2dev = >ps2dev;
>
> - if (IS_SS4PLUS_DEV(priv->dev_id))
> + if (IS_SS4PLUS_DEV(priv->dev_id)) {
> is_dual = (otp[0][0] >> 4) & 0x01;
>
> + if (!is_dual) {
> + /* For support TrackStick of Thinkpad L/E series */
> + if (alps_exit_command_mode(psmouse) == 0 &&
> + alps_enter_command_mode(psmouse) == 0) {
> + reg_val = alps_command_mode_read_reg(psmouse,
> + 0xD7);
> + }
> + alps_exit_command_mode(psmouse);
> + ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> + if (reg_val == 0x0C || reg_val == 0x1D)
> + is_dual = true;
> + }
> + }
> +
> if (is_dual)
> priv->flags |= ALPS_DUALPOINT |
> ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse
> *psmouse,
>
> alps_update_btn_info_ss4_v2(otp, priv);
>
> - alps_update_dual_info_ss4_v2(otp, priv);
> + alps_update_dual_info_ss4_v2(otp, priv, psmouse);
Now looking at this change... Is there reason why you are passing
psmouse parameter there? Because struct alps_data contains psmouse
member.
>
> return 0;
> }
--
Pali Rohár
pali.ro...@gmail.com
Re: Linux & FAT32 label
On Monday 20 November 2017 12:12:56 Karel Zak wrote: > On Sun, Nov 19, 2017 at 01:44:40PM +0100, Pali Rohár wrote: > > On Thursday 09 November 2017 22:21:31 Pali Rohár wrote: > > > So from all tests and discussion I would propose new unification: > > > > > > 1. Read label only from the root directory. If label in root directory > > >is missing then disk would be treated as without label. Label from > > >boot sector would not be read. > > > > > >--> Reason: Windows XP and mlabel ignores what is written in boot > > >sector. Windows XP even do not update boot sector, so label > > >stored in boot sector is incorrect after any change done by > > >Windows XP. > > > > > >This logic is used by all tested MS-DOS and Windows versions, > > >plus also by mtools on Linux. > > > > > > 2. Write label to to both location, boot sector and root directory. > > > > > >--> Reason: MS-DOS 6.22, MS-DOS 7.10, Windows 98 and also mtools on > > >Linux do this. This is also what is written in FAT specification. > > > > > >It also provides backward compatibility with old dosfslabel > > >versions which read label only from boot sector. > > > > > > 2. Process 'NO NAME' label in root directory as 'NO NAME' name. Not > > >as empty label. > > > > > >--> Reason: 'NO NAME' is regular entry in root directory and both > > >Windows XP and mlabel handle it in this way. > > > > > > 3. Process 'NO NAME' label in boot directory as empty label. Not as > > >label with name 'NO NAME'. > > > > > >--> Reason: On Windows XP when formatting empty disk and label is not > > >specified then 'NO NAME' is stored to boot sector. > > > > > >Also in FAT specification is written that empty label is stored > > >as 'NO NAME'. > > > > > > With this change we would get compatibility with MS-DOS, Windows (both > > > DOS-based and NT-based) and also with Linux mtools, modulo problems DOS > > > code page. > > > > > > There are just two negatives: > > > > > > 1) Labels set by old dosfslabel versions (which stored them only to boot > > >sector) would not be visible. But they are already not visible on > > >MS-DOS or Windows machines, and also via mlabel (from mtools). > > > > > > 2) Behavior of blkid and fatlabel would be changed as both tools have > > >different as proposed above, and based on tests they also differ each > > >from other. > > > > > > Andreas, Karel, what do you think about it? > > > > Also for other people, do any have comments on my proposed solution? > > Go ahead and send patch :-) (also with LABEL_FATBOOT=) Now I implemented changes for dosfstools project, pull request is there: https://github.com/dosfstools/dosfstools/pull/73 Just waiting for the Andreas response... Andy, you wanted some manpage update. I did it in this commit: https://github.com/dosfstools/dosfstools/pull/73/commits/3f4f122b7ec8eeb4a0ae0db8e94b8829f51d1163 Can you check if changes in manpage are OK? -- Pali Rohár pali.ro...@gmail.com
Re: Linux & FAT32 label
On Monday 20 November 2017 12:12:56 Karel Zak wrote: > On Sun, Nov 19, 2017 at 01:44:40PM +0100, Pali Rohár wrote: > > On Thursday 09 November 2017 22:21:31 Pali Rohár wrote: > > > So from all tests and discussion I would propose new unification: > > > > > > 1. Read label only from the root directory. If label in root directory > > >is missing then disk would be treated as without label. Label from > > >boot sector would not be read. > > > > > >--> Reason: Windows XP and mlabel ignores what is written in boot > > >sector. Windows XP even do not update boot sector, so label > > >stored in boot sector is incorrect after any change done by > > >Windows XP. > > > > > >This logic is used by all tested MS-DOS and Windows versions, > > >plus also by mtools on Linux. > > > > > > 2. Write label to to both location, boot sector and root directory. > > > > > >--> Reason: MS-DOS 6.22, MS-DOS 7.10, Windows 98 and also mtools on > > >Linux do this. This is also what is written in FAT specification. > > > > > >It also provides backward compatibility with old dosfslabel > > >versions which read label only from boot sector. > > > > > > 2. Process 'NO NAME' label in root directory as 'NO NAME' name. Not > > >as empty label. > > > > > >--> Reason: 'NO NAME' is regular entry in root directory and both > > >Windows XP and mlabel handle it in this way. > > > > > > 3. Process 'NO NAME' label in boot directory as empty label. Not as > > >label with name 'NO NAME'. > > > > > >--> Reason: On Windows XP when formatting empty disk and label is not > > >specified then 'NO NAME' is stored to boot sector. > > > > > >Also in FAT specification is written that empty label is stored > > >as 'NO NAME'. > > > > > > With this change we would get compatibility with MS-DOS, Windows (both > > > DOS-based and NT-based) and also with Linux mtools, modulo problems DOS > > > code page. > > > > > > There are just two negatives: > > > > > > 1) Labels set by old dosfslabel versions (which stored them only to boot > > >sector) would not be visible. But they are already not visible on > > >MS-DOS or Windows machines, and also via mlabel (from mtools). > > > > > > 2) Behavior of blkid and fatlabel would be changed as both tools have > > >different as proposed above, and based on tests they also differ each > > >from other. > > > > > > Andreas, Karel, what do you think about it? > > > > Also for other people, do any have comments on my proposed solution? > > Go ahead and send patch :-) (also with LABEL_FATBOOT=) Now I implemented changes for dosfstools project, pull request is there: https://github.com/dosfstools/dosfstools/pull/73 Just waiting for the Andreas response... Andy, you wanted some manpage update. I did it in this commit: https://github.com/dosfstools/dosfstools/pull/73/commits/3f4f122b7ec8eeb4a0ae0db8e94b8829f51d1163 Can you check if changes in manpage are OK? -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH] Support TrackStick of Thinkpad L570
On Wednesday 29 November 2017 16:01:05 Masaki Ota wrote:
> From: Masaki Ota <masaki@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main
> interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface
> Firmware setting of TrackStick. The detail is that TrackStick otp bit is
> disabled.
> - Add the code that checks 0xD7 address value. This value is device number
> information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the
> value we need to disable Command mode, then we have to enable the device(0xF4
> command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable
> ALPS_DUALPOINT flag.
>
> Signed-off-by: Masaki Ota <masaki@jp.alps.com>
> ---
> drivers/input/mouse/alps.c | 24 +---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..dd2c9e9f4830 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,31 @@ static int alps_update_btn_info_ss4_v2(unsigned char
> otp[][4],
> }
>
> static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -struct alps_data *priv)
> +struct alps_data *priv,
> + struct psmouse *psmouse)
> {
> bool is_dual = false;
> + int reg_val = 0;
> + struct ps2dev *ps2dev = >ps2dev;
>
> - if (IS_SS4PLUS_DEV(priv->dev_id))
> + if (IS_SS4PLUS_DEV(priv->dev_id)) {
> is_dual = (otp[0][0] >> 4) & 0x01;
>
> + if (!is_dual) {
> + /* For support TrackStick of Thinkpad L570 device */
Hi! Jonathan Liu wrote that this patch is needed also for ThinkPad E570p
laptop. So it would be better to add comment that Thinkpad L570 is not
the only one affected and fixing is_dual is needed also for others.
(Probably all Thinkpads which has same generation with this touchpad.)
Such information could prevent in future to wrongly refactor this hook
code.
> + if (alps_exit_command_mode(psmouse) == 0 &&
> + alps_enter_command_mode(psmouse) == 0) {
> + reg_val = alps_command_mode_read_reg(psmouse,
> + 0xD7);
> + }
> + alps_exit_command_mode(psmouse);
> + ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> + if (reg_val == 0x0C || reg_val == 0x1D)
> + is_dual = true;
> + }
> + }
> +
> if (is_dual)
> priv->flags |= ALPS_DUALPOINT |
> ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse
> *psmouse,
>
> alps_update_btn_info_ss4_v2(otp, priv);
>
> - alps_update_dual_info_ss4_v2(otp, priv);
> + alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>
> return 0;
> }
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH] Support TrackStick of Thinkpad L570
On Wednesday 29 November 2017 16:01:05 Masaki Ota wrote:
> From: Masaki Ota
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main
> interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface
> Firmware setting of TrackStick. The detail is that TrackStick otp bit is
> disabled.
> - Add the code that checks 0xD7 address value. This value is device number
> information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the
> value we need to disable Command mode, then we have to enable the device(0xF4
> command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable
> ALPS_DUALPOINT flag.
>
> Signed-off-by: Masaki Ota
> ---
> drivers/input/mouse/alps.c | 24 +---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..dd2c9e9f4830 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,31 @@ static int alps_update_btn_info_ss4_v2(unsigned char
> otp[][4],
> }
>
> static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -struct alps_data *priv)
> +struct alps_data *priv,
> + struct psmouse *psmouse)
> {
> bool is_dual = false;
> + int reg_val = 0;
> + struct ps2dev *ps2dev = >ps2dev;
>
> - if (IS_SS4PLUS_DEV(priv->dev_id))
> + if (IS_SS4PLUS_DEV(priv->dev_id)) {
> is_dual = (otp[0][0] >> 4) & 0x01;
>
> + if (!is_dual) {
> + /* For support TrackStick of Thinkpad L570 device */
Hi! Jonathan Liu wrote that this patch is needed also for ThinkPad E570p
laptop. So it would be better to add comment that Thinkpad L570 is not
the only one affected and fixing is_dual is needed also for others.
(Probably all Thinkpads which has same generation with this touchpad.)
Such information could prevent in future to wrongly refactor this hook
code.
> + if (alps_exit_command_mode(psmouse) == 0 &&
> + alps_enter_command_mode(psmouse) == 0) {
> + reg_val = alps_command_mode_read_reg(psmouse,
> + 0xD7);
> + }
> + alps_exit_command_mode(psmouse);
> + ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> + if (reg_val == 0x0C || reg_val == 0x1D)
> + is_dual = true;
> + }
> + }
> +
> if (is_dual)
> priv->flags |= ALPS_DUALPOINT |
> ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse
> *psmouse,
>
> alps_update_btn_info_ss4_v2(otp, priv);
>
> - alps_update_dual_info_ss4_v2(otp, priv);
> + alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>
> return 0;
> }
--
Pali Rohár
pali.ro...@gmail.com
Re: Linux & FAT32 label
On Monday 20 November 2017 12:12:56 Karel Zak wrote: > Go ahead and send patch :-) (also with LABEL_FATBOOT=) Ok, I prepared patches for util-linux including LABEL_FATBOOT: https://github.com/karelzak/util-linux/compare/master...pali:master I have not created official pull request on github yet as Andreas have not decided yet about dosfstools/fatlabel... -- Pali Rohár pali.ro...@gmail.com
Re: Linux & FAT32 label
On Monday 20 November 2017 12:12:56 Karel Zak wrote: > Go ahead and send patch :-) (also with LABEL_FATBOOT=) Ok, I prepared patches for util-linux including LABEL_FATBOOT: https://github.com/karelzak/util-linux/compare/master...pali:master I have not created official pull request on github yet as Andreas have not decided yet about dosfstools/fatlabel... -- Pali Rohár pali.ro...@gmail.com
Re: i8k_smm_func() takes enormous of time to execute
On Friday 24 November 2017 12:17:30 Oleksandr Natalenko wrote: > > There are two patches waiting to be tested in > > https://bugzilla.kernel.org/show_bug.cgi?id=195751 > > Tested and attached a couple of patches on top of those to the BZ. If > disabling fan control is the only approach here, I can confirm that this > works. Hi! Please figure out which SMM call (they are identified by eax and ebx registers) cause this freeze. So we would now what needs to be blacklisted. -- Pali Rohár pali.ro...@gmail.com
Re: i8k_smm_func() takes enormous of time to execute
On Friday 24 November 2017 12:17:30 Oleksandr Natalenko wrote: > > There are two patches waiting to be tested in > > https://bugzilla.kernel.org/show_bug.cgi?id=195751 > > Tested and attached a couple of patches on top of those to the BZ. If > disabling fan control is the only approach here, I can confirm that this > works. Hi! Please figure out which SMM call (they are identified by eax and ebx registers) cause this freeze. So we would now what needs to be blacklisted. -- Pali Rohár pali.ro...@gmail.com
Re: i8k_smm_func() takes enormous of time to execute
On Thursday 23 November 2017 22:41:03 Oleksandr Natalenko wrote: > Hi, Jonathan, Mario et al. > > I've noticed that querying Dell Vostro 3360 hwmon sensor freezes the system > completely (like, really completely, even mouse cursor does not move, and > sound playback stops) for approx. half of a second. Then, system recovers and > runs as usual. Also, sensor readings are okay, for instance: > > === > $ sensors dell_smm-virtual-0 > dell_smm-virtual-0 > Adapter: Virtual device > Processor Fan: 5615 RPM > CPU:+51.0°C > Ambient:+36.0°C > Other: +63.0°C > SODIMM: +41.0°C > === > > So, I've used trace-cmd to check what takes that much amount of time to > execute, and got this: > > === > # trace-cmd record -p function_graph -l i8k_smm -F sensors dell_smm-virtual-0 > $ trace-cmd report > ... > sensors-23694 [002] 89099.214369: funcgraph_entry: # 503440.746 us | > i8k_smm(); > ... > === > > Clearly, 0.5 s delay. > > Looking at i8k_smm(), it calls i8k_smm_func() on 0th CPU: > > === > 232 ret = smp_call_on_cpu(0, i8k_smm_func, regs, true); > === > > which, in turn, does some asm magic. Hi! If you compile module with DEBUG, then i8k_smm_func function print into dmes how much time it spend in SMM mode. And also it print which SMM action it was. Therefore you would be able to figure out which SMM call (which is just RPC) cause this freeze. Basically when CPU is in SMM mode, then execution of kernel is stopped and kernel does not know that such thing happened. So it is up to the SMM implementation to return control ASAP back to non-SMM mode. Kernel has no control over SMM, it can be fixed only by vendor by new BIOS or firmware update. What we can do is just identify problematic calls and avoid their usage. This problem is independent of kernel or whole operating system. > I know that SMM is kinda "black box", and kernel has little to do with it, > but > I think that under Windows, for instance, it would work without freezes. So, > likely, querying SMM might be done differently. > > I do not know how to approach this issue, thus asking for help/advice. Also, > CCing Jonathan since the comment before asm magic says this: > > === > 137 * Call the System Management Mode BIOS. Code provided by Jonathan > Buzzard. > === > > This was also reported in various places before, for instance, [1], but > unfortunately, without any solution. > > Thanks. > > Regards, > Oleksandr > > [1] https://bugs.launchpad.net/ubuntu/+source/acpi/+bug/10490 -- Pali Rohár pali.ro...@gmail.com
Re: i8k_smm_func() takes enormous of time to execute
On Thursday 23 November 2017 22:41:03 Oleksandr Natalenko wrote: > Hi, Jonathan, Mario et al. > > I've noticed that querying Dell Vostro 3360 hwmon sensor freezes the system > completely (like, really completely, even mouse cursor does not move, and > sound playback stops) for approx. half of a second. Then, system recovers and > runs as usual. Also, sensor readings are okay, for instance: > > === > $ sensors dell_smm-virtual-0 > dell_smm-virtual-0 > Adapter: Virtual device > Processor Fan: 5615 RPM > CPU:+51.0°C > Ambient:+36.0°C > Other: +63.0°C > SODIMM: +41.0°C > === > > So, I've used trace-cmd to check what takes that much amount of time to > execute, and got this: > > === > # trace-cmd record -p function_graph -l i8k_smm -F sensors dell_smm-virtual-0 > $ trace-cmd report > ... > sensors-23694 [002] 89099.214369: funcgraph_entry: # 503440.746 us | > i8k_smm(); > ... > === > > Clearly, 0.5 s delay. > > Looking at i8k_smm(), it calls i8k_smm_func() on 0th CPU: > > === > 232 ret = smp_call_on_cpu(0, i8k_smm_func, regs, true); > === > > which, in turn, does some asm magic. Hi! If you compile module with DEBUG, then i8k_smm_func function print into dmes how much time it spend in SMM mode. And also it print which SMM action it was. Therefore you would be able to figure out which SMM call (which is just RPC) cause this freeze. Basically when CPU is in SMM mode, then execution of kernel is stopped and kernel does not know that such thing happened. So it is up to the SMM implementation to return control ASAP back to non-SMM mode. Kernel has no control over SMM, it can be fixed only by vendor by new BIOS or firmware update. What we can do is just identify problematic calls and avoid their usage. This problem is independent of kernel or whole operating system. > I know that SMM is kinda "black box", and kernel has little to do with it, > but > I think that under Windows, for instance, it would work without freezes. So, > likely, querying SMM might be done differently. > > I do not know how to approach this issue, thus asking for help/advice. Also, > CCing Jonathan since the comment before asm magic says this: > > === > 137 * Call the System Management Mode BIOS. Code provided by Jonathan > Buzzard. > === > > This was also reported in various places before, for instance, [1], but > unfortunately, without any solution. > > Thanks. > > Regards, > Oleksandr > > [1] https://bugs.launchpad.net/ubuntu/+source/acpi/+bug/10490 -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v2] platform/x86: wmi-bmof: New driver to expose embedded Binary WMI MOF metadata
On Tuesday 04 July 2017 15:28:19 Pali Rohár wrote:
> On Tuesday 06 June 2017 22:50:52 Pali Rohár wrote:
> > On Tuesday 06 June 2017 19:02:01 Darren Hart wrote:
> > > On Tue, Jun 06, 2017 at 12:04:40PM +0200, Pali Rohár wrote:
> > > > On Monday 05 June 2017 20:16:44 Andy Lutomirski wrote:
> > > > > +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
> > > > > +MODULE_ALIAS("wmi:" WMI_BMOF_GUID);
> > > >
> > > > Cannot we generate MODULE_ALIAS from module_wmi_driver()? IIRC it
> > > > is working for i2c drivers.
> > >
> > > I could see this being automated since we always use wmi:GUID, but it
> > > isn't currently. Happy to consider it as a follow on.
> > >
> > > Do you have a specific i2c example you think we should consider
> > > following?
> >
> > For i2c you can specify in driver code:
> >
> > MODULE_DEVICE_TABLE(i2c, id_table);
> >
> > And it automatically provides (via file.mod.c) all needed MODULE_ALIAS.
> >
> > So when we have wmi_bmof_id_table in driver, cannot we use this?
> >
> > MODULE_DEVICE_TABLE(wmi, wmi_bmof_id_table);
>
> Just reminder for above idea ↑↑↑
Hi! This email is some months old, so do not know if something was
implemented or not. Does somebody know?
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH v2] platform/x86: wmi-bmof: New driver to expose embedded Binary WMI MOF metadata
On Tuesday 04 July 2017 15:28:19 Pali Rohár wrote:
> On Tuesday 06 June 2017 22:50:52 Pali Rohár wrote:
> > On Tuesday 06 June 2017 19:02:01 Darren Hart wrote:
> > > On Tue, Jun 06, 2017 at 12:04:40PM +0200, Pali Rohár wrote:
> > > > On Monday 05 June 2017 20:16:44 Andy Lutomirski wrote:
> > > > > +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
> > > > > +MODULE_ALIAS("wmi:" WMI_BMOF_GUID);
> > > >
> > > > Cannot we generate MODULE_ALIAS from module_wmi_driver()? IIRC it
> > > > is working for i2c drivers.
> > >
> > > I could see this being automated since we always use wmi:GUID, but it
> > > isn't currently. Happy to consider it as a follow on.
> > >
> > > Do you have a specific i2c example you think we should consider
> > > following?
> >
> > For i2c you can specify in driver code:
> >
> > MODULE_DEVICE_TABLE(i2c, id_table);
> >
> > And it automatically provides (via file.mod.c) all needed MODULE_ALIAS.
> >
> > So when we have wmi_bmof_id_table in driver, cannot we use this?
> >
> > MODULE_DEVICE_TABLE(wmi, wmi_bmof_id_table);
>
> Just reminder for above idea ↑↑↑
Hi! This email is some months old, so do not know if something was
implemented or not. Does somebody know?
--
Pali Rohár
pali.ro...@gmail.com
Re: Linux & FAT32 label
On Wednesday 22 November 2017 12:03:52 Karel Zak wrote: > On Wed, Nov 22, 2017 at 09:52:03AM +0100, Pali Rohár wrote: > > I can prepare patches for blkid (util-linux), but I do not like idea to > > have different algorithm how is read label in fatlabel and blkid. Both > > tools are used by different GUI programs and it is a bad if one GUI > > program on linux (e.g. GParted) tells you that label is A and another > > (e.g. KDE Partition Manager) that label is B. So I rather wait for final > > decision. > > Maybe it's time to update all the GUI stuff to read LABELs and UUIDs > (and another attributes) from udev db. This is just one side of coin. Another is that these GUI applications needs to set/change labels, UUIDs, ... What udev/blkid does not support. -- Pali Rohár pali.ro...@gmail.com
Re: Linux & FAT32 label
On Wednesday 22 November 2017 12:03:52 Karel Zak wrote: > On Wed, Nov 22, 2017 at 09:52:03AM +0100, Pali Rohár wrote: > > I can prepare patches for blkid (util-linux), but I do not like idea to > > have different algorithm how is read label in fatlabel and blkid. Both > > tools are used by different GUI programs and it is a bad if one GUI > > program on linux (e.g. GParted) tells you that label is A and another > > (e.g. KDE Partition Manager) that label is B. So I rather wait for final > > decision. > > Maybe it's time to update all the GUI stuff to read LABELs and UUIDs > (and another attributes) from udev db. This is just one side of coin. Another is that these GUI applications needs to set/change labels, UUIDs, ... What udev/blkid does not support. -- Pali Rohár pali.ro...@gmail.com
Re: Linux & FAT32 label
On Monday 20 November 2017 12:12:56 Karel Zak wrote: > On Sun, Nov 19, 2017 at 01:44:40PM +0100, Pali Rohár wrote: > > On Thursday 09 November 2017 22:21:31 Pali Rohár wrote: > > > So from all tests and discussion I would propose new unification: > > > > > > 1. Read label only from the root directory. If label in root directory > > >is missing then disk would be treated as without label. Label from > > >boot sector would not be read. > > > > > >--> Reason: Windows XP and mlabel ignores what is written in boot > > >sector. Windows XP even do not update boot sector, so label > > >stored in boot sector is incorrect after any change done by > > >Windows XP. > > > > > >This logic is used by all tested MS-DOS and Windows versions, > > >plus also by mtools on Linux. > > > > > > 2. Write label to to both location, boot sector and root directory. > > > > > >--> Reason: MS-DOS 6.22, MS-DOS 7.10, Windows 98 and also mtools on > > >Linux do this. This is also what is written in FAT specification. > > > > > >It also provides backward compatibility with old dosfslabel > > >versions which read label only from boot sector. > > > > > > 2. Process 'NO NAME' label in root directory as 'NO NAME' name. Not > > >as empty label. > > > > > >--> Reason: 'NO NAME' is regular entry in root directory and both > > >Windows XP and mlabel handle it in this way. > > > > > > 3. Process 'NO NAME' label in boot directory as empty label. Not as > > >label with name 'NO NAME'. > > > > > >--> Reason: On Windows XP when formatting empty disk and label is not > > >specified then 'NO NAME' is stored to boot sector. > > > > > >Also in FAT specification is written that empty label is stored > > >as 'NO NAME'. > > > > > > With this change we would get compatibility with MS-DOS, Windows (both > > > DOS-based and NT-based) and also with Linux mtools, modulo problems DOS > > > code page. > > > > > > There are just two negatives: > > > > > > 1) Labels set by old dosfslabel versions (which stored them only to boot > > >sector) would not be visible. But they are already not visible on > > >MS-DOS or Windows machines, and also via mlabel (from mtools). > > > > > > 2) Behavior of blkid and fatlabel would be changed as both tools have > > >different as proposed above, and based on tests they also differ each > > >from other. > > > > > > Andreas, Karel, what do you think about it? > > > > Also for other people, do any have comments on my proposed solution? > > Go ahead and send patch :-) (also with LABEL_FATBOOT=) Ok, but what with fatlabel (from dosfstools)? Andreas, any comments on this solution? I can prepare patches for blkid (util-linux), but I do not like idea to have different algorithm how is read label in fatlabel and blkid. Both tools are used by different GUI programs and it is a bad if one GUI program on linux (e.g. GParted) tells you that label is A and another (e.g. KDE Partition Manager) that label is B. So I rather wait for final decision. -- Pali Rohár pali.ro...@gmail.com
Re: Linux & FAT32 label
On Monday 20 November 2017 12:12:56 Karel Zak wrote: > On Sun, Nov 19, 2017 at 01:44:40PM +0100, Pali Rohár wrote: > > On Thursday 09 November 2017 22:21:31 Pali Rohár wrote: > > > So from all tests and discussion I would propose new unification: > > > > > > 1. Read label only from the root directory. If label in root directory > > >is missing then disk would be treated as without label. Label from > > >boot sector would not be read. > > > > > >--> Reason: Windows XP and mlabel ignores what is written in boot > > >sector. Windows XP even do not update boot sector, so label > > >stored in boot sector is incorrect after any change done by > > >Windows XP. > > > > > >This logic is used by all tested MS-DOS and Windows versions, > > >plus also by mtools on Linux. > > > > > > 2. Write label to to both location, boot sector and root directory. > > > > > >--> Reason: MS-DOS 6.22, MS-DOS 7.10, Windows 98 and also mtools on > > >Linux do this. This is also what is written in FAT specification. > > > > > >It also provides backward compatibility with old dosfslabel > > >versions which read label only from boot sector. > > > > > > 2. Process 'NO NAME' label in root directory as 'NO NAME' name. Not > > >as empty label. > > > > > >--> Reason: 'NO NAME' is regular entry in root directory and both > > >Windows XP and mlabel handle it in this way. > > > > > > 3. Process 'NO NAME' label in boot directory as empty label. Not as > > >label with name 'NO NAME'. > > > > > >--> Reason: On Windows XP when formatting empty disk and label is not > > >specified then 'NO NAME' is stored to boot sector. > > > > > >Also in FAT specification is written that empty label is stored > > >as 'NO NAME'. > > > > > > With this change we would get compatibility with MS-DOS, Windows (both > > > DOS-based and NT-based) and also with Linux mtools, modulo problems DOS > > > code page. > > > > > > There are just two negatives: > > > > > > 1) Labels set by old dosfslabel versions (which stored them only to boot > > >sector) would not be visible. But they are already not visible on > > >MS-DOS or Windows machines, and also via mlabel (from mtools). > > > > > > 2) Behavior of blkid and fatlabel would be changed as both tools have > > >different as proposed above, and based on tests they also differ each > > >from other. > > > > > > Andreas, Karel, what do you think about it? > > > > Also for other people, do any have comments on my proposed solution? > > Go ahead and send patch :-) (also with LABEL_FATBOOT=) Ok, but what with fatlabel (from dosfstools)? Andreas, any comments on this solution? I can prepare patches for blkid (util-linux), but I do not like idea to have different algorithm how is read label in fatlabel and blkid. Both tools are used by different GUI programs and it is a bad if one GUI program on linux (e.g. GParted) tells you that label is A and another (e.g. KDE Partition Manager) that label is B. So I rather wait for final decision. -- Pali Rohár pali.ro...@gmail.com
Re: Dell Vostro 3360 multimedia keys
Hi!
On Tuesday 21 November 2017 15:52:28 Oleksandr Natalenko wrote:
> Hi, Pali et al.
>
> Answers go inline.
>
> On úterý 21. listopadu 2017 14:51:10 CET Pali Rohár wrote:
> > What would happen if you enable wmi_requires_smbios_request?
> > Nothing? And situation is exactly same?
>
> Yes, this is what I did with DMI quirk which enables
> wmi_requires_smbios_request. Keys "1" and "2" behave the same, key
> "3" gets reported via both atkbd and wmi (see my thoughts about this
> key below).
Ok, if wmi_requires_smbios_request is really doing nothing, then it
should not be used. It enables some QSET feature in Dell SMM mode which
is for 2 laptops.
> > > + /* Dell Vostro 3360 combined keycode */
> > > + { KE_KEY,0xe0f1, { KEY_PROG1 } },
> > > + { KE_KEY,0xe0f2, { KEY_PROG2 } },
> >
> > What is purpose of those two keys? Does not linux kernel already
> > have better description for them as KEY_PROG1 and KEY_PROG2?
>
> Well, I didn't find KEY_PROG1/KEY_PROG2 in table. Thus, I just
> added them. Actually, since these keys have some icons drawn on
> them, they have some designated purpose (most likely,
> Windows-related), but I'd rather think of them as of custom
> programmable keys without any specific function assigned.
Looks like that Dell Vostro 3360 have this meaning for multimedia keys:
first key: Mobility Center
second key: Dell Support Center
third key: Dell Instant Launch Manager
(same as other Vostro laptops)
But do not know what KEY_* are used on other Vostro laptops for those
keys. It would be great to use same KEY_*...
> > /* snip */
> >
> > > Third key (the one that is handled via PS/2 keyboard) produces
> > > this in the
> >
> > > kernel buffer:
> > / *snip */
> >
> > Seems you still have not configured keycode for your internal PS/2
> > keyboard to make this key working. Use 'setkeycodes 60 '
> > as written in dmesg.
>
> Well, I've tried. The problem is that if I even configure this key
> via setkeycodes, it doesn't work.
Hm... why it does not work?
CCing Dmitry, can you tell us what needs to be done to "active" key from
AT Keyboard driver which prints following messages to dmesg?
===
Nov 20 15:36:51 spock kernel: atkbd serio0: Unknown key pressed
(translated set 2, code 0x60 on isa0060/serio0).
Nov 20 15:36:51 spock kernel: atkbd serio0: Use 'setkeycodes 60
' to make it known.
===
I was in impression that if we see such message in dmesg, then key is
working fine...
> evtest shows me "value 2" (the
> same value is shown if I press and hold some ordinary key, like "A",
> for instance) instead of 0 and/or 1. Looks like key press event is
> not generated properly by this key via atkbd.
>
> That's why I think wmi_requires_smbios_request quirk must be
> permanently enabled for this laptop, and 3rd key must be handled via
> wmi (like two other keys), not via atkbd. And all atkbd events from
> this key must be just suppressed via userspace.
In dell-wmi we filter all key events which are also delivered via PS/2
AT Keyboard driver. So filtering PS/2 keys instead of WMI is anti-design
solution.
--
Pali Rohár
pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
Re: Dell Vostro 3360 multimedia keys
Hi!
On Tuesday 21 November 2017 15:52:28 Oleksandr Natalenko wrote:
> Hi, Pali et al.
>
> Answers go inline.
>
> On úterý 21. listopadu 2017 14:51:10 CET Pali Rohár wrote:
> > What would happen if you enable wmi_requires_smbios_request?
> > Nothing? And situation is exactly same?
>
> Yes, this is what I did with DMI quirk which enables
> wmi_requires_smbios_request. Keys "1" and "2" behave the same, key
> "3" gets reported via both atkbd and wmi (see my thoughts about this
> key below).
Ok, if wmi_requires_smbios_request is really doing nothing, then it
should not be used. It enables some QSET feature in Dell SMM mode which
is for 2 laptops.
> > > + /* Dell Vostro 3360 combined keycode */
> > > + { KE_KEY,0xe0f1, { KEY_PROG1 } },
> > > + { KE_KEY,0xe0f2, { KEY_PROG2 } },
> >
> > What is purpose of those two keys? Does not linux kernel already
> > have better description for them as KEY_PROG1 and KEY_PROG2?
>
> Well, I didn't find KEY_PROG1/KEY_PROG2 in table. Thus, I just
> added them. Actually, since these keys have some icons drawn on
> them, they have some designated purpose (most likely,
> Windows-related), but I'd rather think of them as of custom
> programmable keys without any specific function assigned.
Looks like that Dell Vostro 3360 have this meaning for multimedia keys:
first key: Mobility Center
second key: Dell Support Center
third key: Dell Instant Launch Manager
(same as other Vostro laptops)
But do not know what KEY_* are used on other Vostro laptops for those
keys. It would be great to use same KEY_*...
> > /* snip */
> >
> > > Third key (the one that is handled via PS/2 keyboard) produces
> > > this in the
> >
> > > kernel buffer:
> > / *snip */
> >
> > Seems you still have not configured keycode for your internal PS/2
> > keyboard to make this key working. Use 'setkeycodes 60 '
> > as written in dmesg.
>
> Well, I've tried. The problem is that if I even configure this key
> via setkeycodes, it doesn't work.
Hm... why it does not work?
CCing Dmitry, can you tell us what needs to be done to "active" key from
AT Keyboard driver which prints following messages to dmesg?
===
Nov 20 15:36:51 spock kernel: atkbd serio0: Unknown key pressed
(translated set 2, code 0x60 on isa0060/serio0).
Nov 20 15:36:51 spock kernel: atkbd serio0: Use 'setkeycodes 60
' to make it known.
===
I was in impression that if we see such message in dmesg, then key is
working fine...
> evtest shows me "value 2" (the
> same value is shown if I press and hold some ordinary key, like "A",
> for instance) instead of 0 and/or 1. Looks like key press event is
> not generated properly by this key via atkbd.
>
> That's why I think wmi_requires_smbios_request quirk must be
> permanently enabled for this laptop, and 3rd key must be handled via
> wmi (like two other keys), not via atkbd. And all atkbd events from
> this key must be just suppressed via userspace.
In dell-wmi we filter all key events which are also delivered via PS/2
AT Keyboard driver. So filtering PS/2 keys instead of WMI is anti-design
solution.
--
Pali Rohár
pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
Re: Dell Vostro 3360 multimedia keys
NF3 | fake code
> ---+--+--
> 0xe0f0 | 0x1 | 0xe0f1
> 0xe0f0 | 0x2 | 0xe0f2
>
> Finally, I add two new fake keycodes to the table, and kernel recognizes
> multimedia keys like this. First key:
>
> ===
> Nov 20 22:24:45 spock kernel: dell_wmi: len: 5, quirk: 1
> Nov 20 22:24:45 spock kernel: dell_wmi: e0f0, 1, e0f1
> ===
>
> Second key:
>
> ===
> Nov 20 22:25:07 spock kernel: dell_wmi: len: 5, quirk: 1
> Nov 20 22:25:07 spock kernel: dell_wmi: e0f0, 2, e0f2
> ===
>
> In evtest I get this:
>
> ===
> Event: time 1511213126.916920, type 4 (EV_MSC), code 4 (MSC_SCAN), value e0f1
> Event: time 1511213126.916920, type 1 (EV_KEY), code 148 (KEY_PROG1), value 1
> Event: time 1511213126.916920, -- SYN_REPORT
> Event: time 1511213126.916944, type 1 (EV_KEY), code 148 (KEY_PROG1), value 0
> Event: time 1511213126.916944, -- SYN_REPORT
>
> Event: time 1511213127.266903, type 4 (EV_MSC), code 4 (MSC_SCAN), value e0f2
> Event: time 1511213127.266903, type 1 (EV_KEY), code 149 (KEY_PROG2), value 1
> Event: time 1511213127.266903, -- SYN_REPORT
> Event: time 1511213127.266919, type 1 (EV_KEY), code 149 (KEY_PROG2), value 0
> Event: time 1511213127.266919, -- SYN_REPORT
> ===
>
> Third key (the one that is handled via PS/2 keyboard) produces this in the
> kernel buffer:
>
> ===
> Nov 20 22:27:08 spock kernel: atkbd serio0: Unknown key pressed (translated
> set 2, code 0x60 on isa0060/serio0).
> Nov 20 22:27:08 spock kernel: atkbd serio0: Use 'setkeycodes 60 ' to
> make it known.
> Nov 20 22:27:08 spock kernel: dell_wmi: len: 3, quirk: 1
> ===
>
> But evtest remains silent.
>
> So, kinda working?
Seems you still have not configured keycode for your internal PS/2
keyboard to make this key working. Use 'setkeycodes 60 ' as
written in dmesg.
> The only question that remains is how to avoid this ugly hack with OR'ing
> multiple values. Apparently, calling dell_wmi_process_key() with only INF2
> DSDT field is insufficient to distinguish two different keys with the same
> code.
After Mario (from @Dell) clarifies how to handle your laptop, I can
write patch for dell-wmi.c driver.
And if there be no statement, then I would try to clean your patch.
There is probably no better option then checking it in buffer/event
handling.
> Also, I've noticed this remark:
>
> ===
> /* Other entries could contain additional information */
> ===
>
> As you may see, they indeed do. Any unimplemented idea behind this statement?
If you look into dell_wmi_keymap_type_[] array there are comments
what is after some key codes. E.g. after key code for brightness up/down
may be value which represent new brightness level. But all those data
are not parsed as dell-wmi does not need it and because those data are
send also via different interfaces (e.g. brightness level via standard
ACPI methods...).
> Any other ideas on how to avoid ugly hackery? Maybe, key table should be
> extended with extra column for INF3 value, and new quirk for this should be
> implemented?
>
> Thanks.
>
> On pondělí 20. listopadu 2017 18:05:50 CET Pali Rohár wrote:
> > Hi!
> >
> > On Monday 20 November 2017 16:08:41 Oleksandr Natalenko wrote:
> > > Hi.
> > >
> > > I've got Dell Vostro 3360 with extra multimedia keys as shown here [1],
> > > but
> > > have no luck to get them working.
> > >
> > > I've modified dell_wmi_smbios_list structure in drivers/platform/x86/dell-
> > > wmi.c adding new entry:
> > >
> > > ===
> > >
> > > 84 {
> > > 85 .callback = dmi_matched,
> > > 86 .ident = "Dell Vostro 3360",
> > > 87 .matches = {
> > > 88 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > > 89 DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 3360"),
> > > 90 },
> > > 91 },
> > >
> > > ===
> >
> > What would happen if you do *not* add this new entry? Please do this
> > test after your notebook was fully turned off to prevent some cached
> > configuration in BIOS.
> >
> > > While pressing keys "1" and/or "2" I get the following notice in dmesg:
> > >
> > > ===
> > > Nov 20 15:53:35 spock kernel: dell_wmi: Unknown key with type 0x and
> > > code 0xe0f0 pressed
> > > ===
> >
> > This means that in dell-wmi.c is missing mapping for code 0xe0f0 in key
> > type 0x.
> >
> >
Re: Dell Vostro 3360 multimedia keys
0xe0f1
> 0xe0f0 | 0x2 | 0xe0f2
>
> Finally, I add two new fake keycodes to the table, and kernel recognizes
> multimedia keys like this. First key:
>
> ===
> Nov 20 22:24:45 spock kernel: dell_wmi: len: 5, quirk: 1
> Nov 20 22:24:45 spock kernel: dell_wmi: e0f0, 1, e0f1
> ===
>
> Second key:
>
> ===
> Nov 20 22:25:07 spock kernel: dell_wmi: len: 5, quirk: 1
> Nov 20 22:25:07 spock kernel: dell_wmi: e0f0, 2, e0f2
> ===
>
> In evtest I get this:
>
> ===
> Event: time 1511213126.916920, type 4 (EV_MSC), code 4 (MSC_SCAN), value e0f1
> Event: time 1511213126.916920, type 1 (EV_KEY), code 148 (KEY_PROG1), value 1
> Event: time 1511213126.916920, -- SYN_REPORT
> Event: time 1511213126.916944, type 1 (EV_KEY), code 148 (KEY_PROG1), value 0
> Event: time 1511213126.916944, -- SYN_REPORT
>
> Event: time 1511213127.266903, type 4 (EV_MSC), code 4 (MSC_SCAN), value e0f2
> Event: time 1511213127.266903, type 1 (EV_KEY), code 149 (KEY_PROG2), value 1
> Event: time 1511213127.266903, -- SYN_REPORT
> Event: time 1511213127.266919, type 1 (EV_KEY), code 149 (KEY_PROG2), value 0
> Event: time 1511213127.266919, -- SYN_REPORT
> ===
>
> Third key (the one that is handled via PS/2 keyboard) produces this in the
> kernel buffer:
>
> ===
> Nov 20 22:27:08 spock kernel: atkbd serio0: Unknown key pressed (translated
> set 2, code 0x60 on isa0060/serio0).
> Nov 20 22:27:08 spock kernel: atkbd serio0: Use 'setkeycodes 60 ' to
> make it known.
> Nov 20 22:27:08 spock kernel: dell_wmi: len: 3, quirk: 1
> ===
>
> But evtest remains silent.
>
> So, kinda working?
Seems you still have not configured keycode for your internal PS/2
keyboard to make this key working. Use 'setkeycodes 60 ' as
written in dmesg.
> The only question that remains is how to avoid this ugly hack with OR'ing
> multiple values. Apparently, calling dell_wmi_process_key() with only INF2
> DSDT field is insufficient to distinguish two different keys with the same
> code.
After Mario (from @Dell) clarifies how to handle your laptop, I can
write patch for dell-wmi.c driver.
And if there be no statement, then I would try to clean your patch.
There is probably no better option then checking it in buffer/event
handling.
> Also, I've noticed this remark:
>
> ===
> /* Other entries could contain additional information */
> ===
>
> As you may see, they indeed do. Any unimplemented idea behind this statement?
If you look into dell_wmi_keymap_type_[] array there are comments
what is after some key codes. E.g. after key code for brightness up/down
may be value which represent new brightness level. But all those data
are not parsed as dell-wmi does not need it and because those data are
send also via different interfaces (e.g. brightness level via standard
ACPI methods...).
> Any other ideas on how to avoid ugly hackery? Maybe, key table should be
> extended with extra column for INF3 value, and new quirk for this should be
> implemented?
>
> Thanks.
>
> On pondělí 20. listopadu 2017 18:05:50 CET Pali Rohár wrote:
> > Hi!
> >
> > On Monday 20 November 2017 16:08:41 Oleksandr Natalenko wrote:
> > > Hi.
> > >
> > > I've got Dell Vostro 3360 with extra multimedia keys as shown here [1],
> > > but
> > > have no luck to get them working.
> > >
> > > I've modified dell_wmi_smbios_list structure in drivers/platform/x86/dell-
> > > wmi.c adding new entry:
> > >
> > > ===
> > >
> > > 84 {
> > > 85 .callback = dmi_matched,
> > > 86 .ident = "Dell Vostro 3360",
> > > 87 .matches = {
> > > 88 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > > 89 DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 3360"),
> > > 90 },
> > > 91 },
> > >
> > > ===
> >
> > What would happen if you do *not* add this new entry? Please do this
> > test after your notebook was fully turned off to prevent some cached
> > configuration in BIOS.
> >
> > > While pressing keys "1" and/or "2" I get the following notice in dmesg:
> > >
> > > ===
> > > Nov 20 15:53:35 spock kernel: dell_wmi: Unknown key with type 0x and
> > > code 0xe0f0 pressed
> > > ===
> >
> > This means that in dell-wmi.c is missing mapping for code 0xe0f0 in key
> > type 0x.
> >
> > Find array dell_wmi_keymap_type_[] and add there line:
> >
> > { KE_KEY, 0xe0f0, { KEY_someth
Re: Dell Vostro 3360 multimedia keys
value e025
> Event: time 1511189973.016907, type 1 (EV_KEY), code 203 (KEY_PROG4), value 1
> Event: time 1511189973.016907, -- SYN_REPORT
> Event: time 1511189973.016942, type 1 (EV_KEY), code 203 (KEY_PROG4), value 0
> Event: time 1511189973.016942, -- SYN_REPORT
> ===
So it means that third key is also received by dell-wmi? Anyway see
function dell_wmi_process_key() and line:
if (type == 0x && code == 0xe025 && !wmi_requires_smbios_request)
return;
It is there just to ensure that key is not send via both PS/2 keyboard
and dell-wmi.
So I guess you should disable wmi_requires_smbios_request.
> I think this corresponds to what I see in drivers/platform/x86/dell-wmi.c
> file
> here:
>
> ===
> 143 /* Dell Instant Launch key */
> 144 { KE_KEY,0xe025, { KEY_PROG4 } },
> ===
>
> Other two keys do not produce any evtest output.
>
> Here is acpidump output: [2]
> Here is decompiled DSDT: [3]
>
> Also, I've raised this question before a couple of times (for instance, [4]),
> but unfortunately got no result :(.
>
> Could you please help me in fixing multimedia keys?
I Hope that this email helps you.
> Thanks.
>
> [1] http://beta.hstor.org/files/c3b/a26/628/
> c3ba26628409486f8b9ae16d97be7d21.jpg
> [2] https://gist.github.com/7c04035ba2a3f0e5501af83efdb1456d
> [3] https://gist.github.com/83687126c46417b5bc0b48529de52460
> [4] https://www.spinics.net/lists/platform-driver-x86/msg05251.html
--
Pali Rohár
pali.ro...@gmail.com
Re: Dell Vostro 3360 multimedia keys
value e025
> Event: time 1511189973.016907, type 1 (EV_KEY), code 203 (KEY_PROG4), value 1
> Event: time 1511189973.016907, -- SYN_REPORT
> Event: time 1511189973.016942, type 1 (EV_KEY), code 203 (KEY_PROG4), value 0
> Event: time 1511189973.016942, -- SYN_REPORT
> ===
So it means that third key is also received by dell-wmi? Anyway see
function dell_wmi_process_key() and line:
if (type == 0x && code == 0xe025 && !wmi_requires_smbios_request)
return;
It is there just to ensure that key is not send via both PS/2 keyboard
and dell-wmi.
So I guess you should disable wmi_requires_smbios_request.
> I think this corresponds to what I see in drivers/platform/x86/dell-wmi.c
> file
> here:
>
> ===
> 143 /* Dell Instant Launch key */
> 144 { KE_KEY,0xe025, { KEY_PROG4 } },
> ===
>
> Other two keys do not produce any evtest output.
>
> Here is acpidump output: [2]
> Here is decompiled DSDT: [3]
>
> Also, I've raised this question before a couple of times (for instance, [4]),
> but unfortunately got no result :(.
>
> Could you please help me in fixing multimedia keys?
I Hope that this email helps you.
> Thanks.
>
> [1] http://beta.hstor.org/files/c3b/a26/628/
> c3ba26628409486f8b9ae16d97be7d21.jpg
> [2] https://gist.github.com/7c04035ba2a3f0e5501af83efdb1456d
> [3] https://gist.github.com/83687126c46417b5bc0b48529de52460
> [4] https://www.spinics.net/lists/platform-driver-x86/msg05251.html
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH] Support TrackStick of Thinkpad L570
On Monday 20 November 2017 16:55:30 Masaki Ota wrote:
> From: Masaki Ota <masaki@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main
> interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface
> Firmware setting of TrackStick. The detail is that TrackStick otp bit is
> disabled.
> - Add the code that checks 0xD7 address value. This value is device number
> information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the
> value we need to disable Command mode, then we have to enable the device(0xF4
> command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable
> ALPS_DUALPOINT flag.
So, the root of this problem is in ALPS firmware which provides wrong
information to kernel?
Masaki, I have two questions:
1) Can ALPS or Lenovo release a new firmware update for this Thinkpad to
fix this issue?
2) Have all Thinkpad L570 machines trackpoint?
Dmitry, as a workaround for firmware bug on particular notebook, would
not be better to check DMI information and DMI based hook?
> Signed-off-by: Masaki Ota <masaki@jp.alps.com>
> ---
> drivers/input/mouse/alps.c | 21 ++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..cce52104ed5a 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,28 @@ static int alps_update_btn_info_ss4_v2(unsigned char
> otp[][4],
> }
>
> static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -struct alps_data *priv)
> +struct alps_data *priv,
> + struct psmouse *psmouse)
> {
> bool is_dual = false;
> + int reg_val = 0;
> + struct ps2dev *ps2dev = >ps2dev;
>
> - if (IS_SS4PLUS_DEV(priv->dev_id))
> + if (IS_SS4PLUS_DEV(priv->dev_id)) {
> is_dual = (otp[0][0] >> 4) & 0x01;
>
> + /* For support TrackStick of Thinkpad L570 device */
> + if (alps_exit_command_mode(psmouse) == 0 &&
> + alps_enter_command_mode(psmouse) == 0) {
> + reg_val = alps_command_mode_read_reg(psmouse, 0xD7);
> + }
> + alps_exit_command_mode(psmouse);
> + ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> + if (reg_val == 0x0C || reg_val == 0x1D)
> + is_dual = true;
> + }
> +
> if (is_dual)
> priv->flags |= ALPS_DUALPOINT |
> ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2585,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse
> *psmouse,
>
> alps_update_btn_info_ss4_v2(otp, priv);
>
> - alps_update_dual_info_ss4_v2(otp, priv);
> + alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>
> return 0;
> }
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH] Support TrackStick of Thinkpad L570
On Monday 20 November 2017 16:55:30 Masaki Ota wrote:
> From: Masaki Ota
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main
> interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface
> Firmware setting of TrackStick. The detail is that TrackStick otp bit is
> disabled.
> - Add the code that checks 0xD7 address value. This value is device number
> information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the
> value we need to disable Command mode, then we have to enable the device(0xF4
> command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable
> ALPS_DUALPOINT flag.
So, the root of this problem is in ALPS firmware which provides wrong
information to kernel?
Masaki, I have two questions:
1) Can ALPS or Lenovo release a new firmware update for this Thinkpad to
fix this issue?
2) Have all Thinkpad L570 machines trackpoint?
Dmitry, as a workaround for firmware bug on particular notebook, would
not be better to check DMI information and DMI based hook?
> Signed-off-by: Masaki Ota
> ---
> drivers/input/mouse/alps.c | 21 ++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..cce52104ed5a 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,28 @@ static int alps_update_btn_info_ss4_v2(unsigned char
> otp[][4],
> }
>
> static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -struct alps_data *priv)
> +struct alps_data *priv,
> + struct psmouse *psmouse)
> {
> bool is_dual = false;
> + int reg_val = 0;
> + struct ps2dev *ps2dev = >ps2dev;
>
> - if (IS_SS4PLUS_DEV(priv->dev_id))
> + if (IS_SS4PLUS_DEV(priv->dev_id)) {
> is_dual = (otp[0][0] >> 4) & 0x01;
>
> + /* For support TrackStick of Thinkpad L570 device */
> + if (alps_exit_command_mode(psmouse) == 0 &&
> + alps_enter_command_mode(psmouse) == 0) {
> + reg_val = alps_command_mode_read_reg(psmouse, 0xD7);
> + }
> + alps_exit_command_mode(psmouse);
> + ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> + if (reg_val == 0x0C || reg_val == 0x1D)
> + is_dual = true;
> + }
> +
> if (is_dual)
> priv->flags |= ALPS_DUALPOINT |
> ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2585,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse
> *psmouse,
>
> alps_update_btn_info_ss4_v2(otp, priv);
>
> - alps_update_dual_info_ss4_v2(otp, priv);
> + alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>
> return 0;
> }
--
Pali Rohár
pali.ro...@gmail.com
Re: Linux & FAT32 label
On Thursday 09 November 2017 22:21:31 Pali Rohár wrote: > On Sunday 05 November 2017 14:06:08 Pali Rohár wrote: > > On Wednesday 11 October 2017 23:24:35 Pali Rohár wrote: > > > On Wednesday 04 October 2017 17:33:32 Pali Rohár wrote: > > > > Hi! There is a big inconsistency in Linux tools which read or write > > > > FAT32 label in filesystem images. The most common used are tools: > > > > blkid (from util-linux project), fatlabel (previously known as > > > > dosfslabel; from dosfstools project) and mlabel (from mtools project). > > > > > > > > FAT32 is itself a big mess from Microsoft hell and even FAT32 > > > > implementation in Microsoft Windows systems is not compliant to the > > > > released FAT32 documentation from Microsoft. > > > > > > > > In past months I observed that Linux FAT32 tools has its own way how > > > > they interpret FAT32 label (known as volume id) and because every GUI > > > > application uses one of those low-level command line tool, it is a big > > > > mess if one application say that FAT32 label is A and another that it is > > > > B. And then Windows XP say, it is C. > > > > > > > > I would like to open discussion if it would be possible to change > > > > behavior how blkid (from util-linux project) and fatlabel (from > > > > dosfstool project) handle FAT32 label. Ideally to report exactly same > > > > output. > > > > > > > > Basic information about FAT32 label: > > > > > > > > 1) It is stored in two locations: boot sector and root directory as > > > >file name. > > > > > > > > 2) In both location format is 11 bytes, padded with spaces (not nulls). > > > > > > > > 3) Empty label in boot sector is stored as "NO NAME" and not as > > > >empty string. > > > > > > > > 4) Empty label in root directory is stored either as name which starts > > > >with byte 0xE5, or is not stored in root directory at all. > > > > > > > > 5) If label contains leading byte 0xE5, then in root directory is stored > > > >as byte 0x05. > > > > > > > > 6) Label string is stored according to current DOS code page. Therefore > > > >label string needs to be converted to bytes. > > > > > > > > 7) Label string cannot contain control characters and characters from > > > >the set ? / \ | . , ; : + = [ ] < > " plus lower case characters > > > >are stored as their upper case variant (not only ASCII). > > > > > > > > (Please correct me if I'm wrong in some of those points) > > > > > > > > Plus Microsoft Windows systems fully ignores label stored in boot > > > > sector. Seems they do not read it nor they do not update it on changes. > > > > > > > > Looks like that mlabel (from mtools) applies all above rules and uses > > > > DOS code page 850 by default (can be changed in config file). > > > > > > > > blkid and fatlabel process special cases from 1) to 5) differently and > > > > they operates on raw bytes, not strings (in DOS code page). > > > > > > > > mlabel reads label from the root directory (missing entry is interpreted > > > > as no label; there is no fallback to boot sector), but "set" operation > > > > modify label in both location boot sector + root directory. Basically it > > > > is near to Windows implementation. And reason why Gparted GUI > > > > application uses mlabel and not fatlabel. > > > > > > > > As Linux does not have "current DOS code page" and argv arguments are > > > > not (Unicode) strings, but arbitrary bytes, I understand that for point > > > > 6) it is easier to operates not on FAT strings (in current code page), > > > > but rather on bytes. Which also would be same on all machines with any > > > > configuration. > > > > > > > > But would it be possible to decide and unify handling of point 2), 3), > > > > 4), 5)? Ideally with combination how to handle situation when different > > > > label is stored in boot sector and root directory. > > > > > > > > As Windows does not use label in boot sector, it is very common > > > > situation that label in boot sector differs from the root directory. > > > > > > > > The bes
Re: Linux & FAT32 label
On Thursday 09 November 2017 22:21:31 Pali Rohár wrote: > On Sunday 05 November 2017 14:06:08 Pali Rohár wrote: > > On Wednesday 11 October 2017 23:24:35 Pali Rohár wrote: > > > On Wednesday 04 October 2017 17:33:32 Pali Rohár wrote: > > > > Hi! There is a big inconsistency in Linux tools which read or write > > > > FAT32 label in filesystem images. The most common used are tools: > > > > blkid (from util-linux project), fatlabel (previously known as > > > > dosfslabel; from dosfstools project) and mlabel (from mtools project). > > > > > > > > FAT32 is itself a big mess from Microsoft hell and even FAT32 > > > > implementation in Microsoft Windows systems is not compliant to the > > > > released FAT32 documentation from Microsoft. > > > > > > > > In past months I observed that Linux FAT32 tools has its own way how > > > > they interpret FAT32 label (known as volume id) and because every GUI > > > > application uses one of those low-level command line tool, it is a big > > > > mess if one application say that FAT32 label is A and another that it is > > > > B. And then Windows XP say, it is C. > > > > > > > > I would like to open discussion if it would be possible to change > > > > behavior how blkid (from util-linux project) and fatlabel (from > > > > dosfstool project) handle FAT32 label. Ideally to report exactly same > > > > output. > > > > > > > > Basic information about FAT32 label: > > > > > > > > 1) It is stored in two locations: boot sector and root directory as > > > >file name. > > > > > > > > 2) In both location format is 11 bytes, padded with spaces (not nulls). > > > > > > > > 3) Empty label in boot sector is stored as "NO NAME" and not as > > > >empty string. > > > > > > > > 4) Empty label in root directory is stored either as name which starts > > > >with byte 0xE5, or is not stored in root directory at all. > > > > > > > > 5) If label contains leading byte 0xE5, then in root directory is stored > > > >as byte 0x05. > > > > > > > > 6) Label string is stored according to current DOS code page. Therefore > > > >label string needs to be converted to bytes. > > > > > > > > 7) Label string cannot contain control characters and characters from > > > >the set ? / \ | . , ; : + = [ ] < > " plus lower case characters > > > >are stored as their upper case variant (not only ASCII). > > > > > > > > (Please correct me if I'm wrong in some of those points) > > > > > > > > Plus Microsoft Windows systems fully ignores label stored in boot > > > > sector. Seems they do not read it nor they do not update it on changes. > > > > > > > > Looks like that mlabel (from mtools) applies all above rules and uses > > > > DOS code page 850 by default (can be changed in config file). > > > > > > > > blkid and fatlabel process special cases from 1) to 5) differently and > > > > they operates on raw bytes, not strings (in DOS code page). > > > > > > > > mlabel reads label from the root directory (missing entry is interpreted > > > > as no label; there is no fallback to boot sector), but "set" operation > > > > modify label in both location boot sector + root directory. Basically it > > > > is near to Windows implementation. And reason why Gparted GUI > > > > application uses mlabel and not fatlabel. > > > > > > > > As Linux does not have "current DOS code page" and argv arguments are > > > > not (Unicode) strings, but arbitrary bytes, I understand that for point > > > > 6) it is easier to operates not on FAT strings (in current code page), > > > > but rather on bytes. Which also would be same on all machines with any > > > > configuration. > > > > > > > > But would it be possible to decide and unify handling of point 2), 3), > > > > 4), 5)? Ideally with combination how to handle situation when different > > > > label is stored in boot sector and root directory. > > > > > > > > As Windows does not use label in boot sector, it is very common > > > > situation that label in boot sector differs from the root directory. > > > > > > > > The bes
Re: [PATCH] Input: ALPS - fix DualPoint flag for 74 03 28 devices
On Wednesday 15 November 2017 14:34:04 Aaron Ma wrote:
> There is a regression of commit 4a646580f793 ("Input: ALPS - fix
> two-finger scroll breakage"), ALPS device fails with log:
>
> psmouse serio1: alps: Rejected trackstick packet from non DualPoint device
>
> ALPS device with id "74 03 28" report OTP[0] data 0xCE after
> commit 4a646580f793, after restore the OTP reading order,
> it becomes to 0x10 as before and reports the right flag.
>
> Fixes: 4a646580f793 ("Input: ALPS - fix two-finger scroll breakage")
> Cc: <sta...@vger.kernel.org>
> Signed-off-by: Aaron Ma <aaron...@canonical.com>
> ---
> drivers/input/mouse/alps.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 579b899add26..c59b8f7ca2fc 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2562,8 +2562,8 @@ static int alps_set_defaults_ss4_v2(struct psmouse
> *psmouse,
>
> memset(otp, 0, sizeof(otp));
>
> - if (alps_get_otp_values_ss4_v2(psmouse, 1, [1][0]) ||
> - alps_get_otp_values_ss4_v2(psmouse, 0, [0][0]))
> + if (alps_get_otp_values_ss4_v2(psmouse, 0, [0][0]) ||
> + alps_get_otp_values_ss4_v2(psmouse, 1, [1][0]))
> return -1;
>
> alps_update_device_area_ss4_v2(otp, priv);
Masaki Ota, please look at this patch as it partially revert your commit
4a646580f793 ("Input: ALPS - fix two-finger scroll breakage"). Something
smells here.
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH] Input: ALPS - fix DualPoint flag for 74 03 28 devices
On Wednesday 15 November 2017 14:34:04 Aaron Ma wrote:
> There is a regression of commit 4a646580f793 ("Input: ALPS - fix
> two-finger scroll breakage"), ALPS device fails with log:
>
> psmouse serio1: alps: Rejected trackstick packet from non DualPoint device
>
> ALPS device with id "74 03 28" report OTP[0] data 0xCE after
> commit 4a646580f793, after restore the OTP reading order,
> it becomes to 0x10 as before and reports the right flag.
>
> Fixes: 4a646580f793 ("Input: ALPS - fix two-finger scroll breakage")
> Cc:
> Signed-off-by: Aaron Ma
> ---
> drivers/input/mouse/alps.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 579b899add26..c59b8f7ca2fc 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2562,8 +2562,8 @@ static int alps_set_defaults_ss4_v2(struct psmouse
> *psmouse,
>
> memset(otp, 0, sizeof(otp));
>
> - if (alps_get_otp_values_ss4_v2(psmouse, 1, [1][0]) ||
> - alps_get_otp_values_ss4_v2(psmouse, 0, [0][0]))
> + if (alps_get_otp_values_ss4_v2(psmouse, 0, [0][0]) ||
> + alps_get_otp_values_ss4_v2(psmouse, 1, [1][0]))
> return -1;
>
> alps_update_device_area_ss4_v2(otp, priv);
Masaki Ota, please look at this patch as it partially revert your commit
4a646580f793 ("Input: ALPS - fix two-finger scroll breakage"). Something
smells here.
--
Pali Rohár
pali.ro...@gmail.com
Re: N900 kernel support
On Friday 10 November 2017 21:54:08 Pavel Machek wrote: > Hi! > > > Hi! Yesterday I updated https://elinux.org/N900 page based on data which > > I found in linux git repository. So driver status should be > > up-to-date. > > BTW... do you have any reasonable userland to run on n900? Maemo 5 with some modifications worked with some 4.x kernels (probably 4.6 or 4.7). I have not tested recent versions. And Maemo 5 is usable userland, just still closed... > I'm currently playing with postmarketos -- > https://wiki.postmarketos.org/wiki/Nokia_N900_(nokia-rx51) -- lots of > fun, but still a lot of work to do > > Best regards, > Pavel -- Pali Rohár pali.ro...@gmail.com
Re: N900 kernel support
On Friday 10 November 2017 21:54:08 Pavel Machek wrote: > Hi! > > > Hi! Yesterday I updated https://elinux.org/N900 page based on data which > > I found in linux git repository. So driver status should be > > up-to-date. > > BTW... do you have any reasonable userland to run on n900? Maemo 5 with some modifications worked with some 4.x kernels (probably 4.6 or 4.7). I have not tested recent versions. And Maemo 5 is usable userland, just still closed... > I'm currently playing with postmarketos -- > https://wiki.postmarketos.org/wiki/Nokia_N900_(nokia-rx51) -- lots of > fun, but still a lot of work to do > > Best regards, > Pavel -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v3] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410
On Sunday 12 November 2017 10:01:10 Gabriel M. Elder wrote: > If necessary, I am willing to test whatever final proposed patch the > community recommends, settles on, and accepts. Would you all like me to > test the v3 patch as well? v3 has just renamed variable, so I do not think that it is needed to test it again, if you have already tested v2. But of course you can test v3... > Or are we calling it good at this point, and > can expect this to be merged into a final stable kernel release sometime > soon? I must admit, I'm looking forward to officially making this bug go > bye-bye! -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v3] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410
On Sunday 12 November 2017 10:01:10 Gabriel M. Elder wrote: > If necessary, I am willing to test whatever final proposed patch the > community recommends, settles on, and accepts. Would you all like me to > test the v3 patch as well? v3 has just renamed variable, so I do not think that it is needed to test it again, if you have already tested v2. But of course you can test v3... > Or are we calling it good at this point, and > can expect this to be merged into a final stable kernel release sometime > soon? I must admit, I'm looking forward to officially making this bug go > bye-bye! -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH] omap3-n900: enable autofocus
On Sunday 12 November 2017 12:25:29 Pavel Machek wrote:
>
> Connect auto-focus coil to the camera sensor, so that it can be used
> by camera applications.
>
> Signed-off-by: Pavel Machek <pa...@ucw.cz>
Acked-by: Pali Rohár <pali.ro...@gmail.com>
> diff --git a/arch/arm/boot/dts/omap3-n900.dts
> b/arch/arm/boot/dts/omap3-n900.dts
> index 669c51c..59cc743 100644
> --- a/arch/arm/boot/dts/omap3-n900.dts
> +++ b/arch/arm/boot/dts/omap3-n900.dts
> @@ -778,6 +778,8 @@
>
> reset-gpio = < 6 GPIO_ACTIVE_HIGH>; /* 102 */
>
> + lens-focus = <>;
> +
> port {
> csi_cam1: endpoint {
> bus-type = <3>; /* CCP2 */
>
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH] omap3-n900: enable autofocus
On Sunday 12 November 2017 12:25:29 Pavel Machek wrote:
>
> Connect auto-focus coil to the camera sensor, so that it can be used
> by camera applications.
>
> Signed-off-by: Pavel Machek
Acked-by: Pali Rohár
> diff --git a/arch/arm/boot/dts/omap3-n900.dts
> b/arch/arm/boot/dts/omap3-n900.dts
> index 669c51c..59cc743 100644
> --- a/arch/arm/boot/dts/omap3-n900.dts
> +++ b/arch/arm/boot/dts/omap3-n900.dts
> @@ -778,6 +778,8 @@
>
> reset-gpio = < 6 GPIO_ACTIVE_HIGH>; /* 102 */
>
> + lens-focus = <>;
> +
> port {
> csi_cam1: endpoint {
> bus-type = <3>; /* CCP2 */
>
--
Pali Rohár
pali.ro...@gmail.com
[PATCH] dell-laptop: Use bool type in struct quirk_entry for true/false fields
In struct quirk_entry some boolean fields used int, some u8 type. Change
them all to bool type.
Signed-off-by: Pali Rohár <pali.ro...@gmail.com>
---
drivers/platform/x86/dell-laptop.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/x86/dell-laptop.c
b/drivers/platform/x86/dell-laptop.c
index 7424e53..de7d04b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -48,10 +48,10 @@
#define KBD_LED_AC_TOKEN 0x0451
struct quirk_entry {
- u8 touchpad_led;
- u8 kbd_led_levels_off_1;
+ bool touchpad_led;
+ bool kbd_led_levels_off_1;
- int needs_kbd_timeouts;
+ bool needs_kbd_timeouts;
/*
* Ordered list of timeouts expressed in seconds.
* The list must end with -1
@@ -62,7 +62,7 @@ struct quirk_entry {
static struct quirk_entry *quirks;
static struct quirk_entry quirk_dell_vostro_v130 = {
- .touchpad_led = 1,
+ .touchpad_led = true,
};
static int __init dmi_matched(const struct dmi_system_id *dmi)
@@ -76,12 +76,12 @@ static int __init dmi_matched(const struct dmi_system_id
*dmi)
* is used then BIOS silently set timeout to 0 without any error message.
*/
static struct quirk_entry quirk_dell_xps13_9333 = {
- .needs_kbd_timeouts = 1,
+ .needs_kbd_timeouts = true,
.kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 },
};
static struct quirk_entry quirk_dell_latitude_e6410 = {
- .kbd_led_levels_off_1 = 1,
+ .kbd_led_levels_off_1 = true,
};
static struct platform_driver platform_driver = {
--
1.7.9.5
[PATCH] dell-laptop: Use bool type in struct quirk_entry for true/false fields
In struct quirk_entry some boolean fields used int, some u8 type. Change
them all to bool type.
Signed-off-by: Pali Rohár
---
drivers/platform/x86/dell-laptop.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/x86/dell-laptop.c
b/drivers/platform/x86/dell-laptop.c
index 7424e53..de7d04b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -48,10 +48,10 @@
#define KBD_LED_AC_TOKEN 0x0451
struct quirk_entry {
- u8 touchpad_led;
- u8 kbd_led_levels_off_1;
+ bool touchpad_led;
+ bool kbd_led_levels_off_1;
- int needs_kbd_timeouts;
+ bool needs_kbd_timeouts;
/*
* Ordered list of timeouts expressed in seconds.
* The list must end with -1
@@ -62,7 +62,7 @@ struct quirk_entry {
static struct quirk_entry *quirks;
static struct quirk_entry quirk_dell_vostro_v130 = {
- .touchpad_led = 1,
+ .touchpad_led = true,
};
static int __init dmi_matched(const struct dmi_system_id *dmi)
@@ -76,12 +76,12 @@ static int __init dmi_matched(const struct dmi_system_id
*dmi)
* is used then BIOS silently set timeout to 0 without any error message.
*/
static struct quirk_entry quirk_dell_xps13_9333 = {
- .needs_kbd_timeouts = 1,
+ .needs_kbd_timeouts = true,
.kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 },
};
static struct quirk_entry quirk_dell_latitude_e6410 = {
- .kbd_led_levels_off_1 = 1,
+ .kbd_led_levels_off_1 = true,
};
static struct platform_driver platform_driver = {
--
1.7.9.5
Re: [PATCH v3] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410
On Thursday 02 November 2017 18:18:43 Darren Hart wrote:
> On Thu, Nov 02, 2017 at 09:25:24PM +0100, Pali Rohár wrote:
> > This machine reports number of keyboard backlight led levels, instead of
> > value of the last led level index. Therefore max_brightness properly needs
> > to be subtracted by 1 to match led max_brightness API.
> >
> > Signed-off-by: Pali Rohár <pali.ro...@gmail.com>
> > Reported-by: Gabriel M. Elder <gabr...@tekgnowsys.com>
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=196913
> > ---
> > Changes since v2:
> > * Rename quirk entry to kbd_led_levels_off_1
> > Changes since v1:
> > * Update kbd_info.levels at initialization time based on quirk
> > ---
> > drivers/platform/x86/dell-laptop.c | 17 +
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/platform/x86/dell-laptop.c
> > b/drivers/platform/x86/dell-laptop.c
> > index f42159f..7424e53 100644
> > --- a/drivers/platform/x86/dell-laptop.c
> > +++ b/drivers/platform/x86/dell-laptop.c
> > @@ -49,6 +49,7 @@
> >
> > struct quirk_entry {
> > u8 touchpad_led;
> > + u8 kbd_led_levels_off_1;
>
> I believe you and Andy agreed to use a boolean type here?
I'm going to fix this and other entries to boolean type in another
patch.
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH v3] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410
On Thursday 02 November 2017 18:18:43 Darren Hart wrote:
> On Thu, Nov 02, 2017 at 09:25:24PM +0100, Pali Rohár wrote:
> > This machine reports number of keyboard backlight led levels, instead of
> > value of the last led level index. Therefore max_brightness properly needs
> > to be subtracted by 1 to match led max_brightness API.
> >
> > Signed-off-by: Pali Rohár
> > Reported-by: Gabriel M. Elder
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=196913
> > ---
> > Changes since v2:
> > * Rename quirk entry to kbd_led_levels_off_1
> > Changes since v1:
> > * Update kbd_info.levels at initialization time based on quirk
> > ---
> > drivers/platform/x86/dell-laptop.c | 17 +
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/platform/x86/dell-laptop.c
> > b/drivers/platform/x86/dell-laptop.c
> > index f42159f..7424e53 100644
> > --- a/drivers/platform/x86/dell-laptop.c
> > +++ b/drivers/platform/x86/dell-laptop.c
> > @@ -49,6 +49,7 @@
> >
> > struct quirk_entry {
> > u8 touchpad_led;
> > + u8 kbd_led_levels_off_1;
>
> I believe you and Andy agreed to use a boolean type here?
I'm going to fix this and other entries to boolean type in another
patch.
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function
On Friday 10 November 2017 21:26:01 Luis R. Rodriguez wrote:
> On Fri, Nov 10, 2017 at 12:38:27AM +0100, Pali Rohár wrote:
> > This function works pretty much like request_firmware(), but it prefer
> > usermode helper. If usermode helper fails then it fallback to direct
> > access. Useful for dynamic or model specific firmware data.
> >
> > Signed-off-by: Pali Rohár <pali.ro...@gmail.com>
> > ---
> > drivers/base/firmware_class.c | 45
> > +++--
> > include/linux/firmware.h |9 +
> > 2 files changed, 52 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 4b57cf5..c3a9fe5 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st,
> > enum fw_status status)
> > #endif
> > #define FW_OPT_NO_WARN (1U << 3)
> > #define FW_OPT_NOCACHE (1U << 4)
> > +#ifdef CONFIG_FW_LOADER_USER_HELPER
> > +#define FW_OPT_PREFER_USER (1U << 5)
> > +#else
> > +#define FW_OPT_PREFER_USER 0
> > +#endif
>
> I've been cleaning these up these flags [0], which I'll shortly respin based
> on feedback, so this sort of stuff should be avoided at all costs.
>
> Regardless of this even if you *leave* the flag in place and a driver required
> this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
> calling fw_load_from_user_helper would just already return -ENOENT, as such it
> would in turn fallback to direct fs loading so the #ifef'ery seems to be not
> needed.
>
> [0] https://lkml.kernel.org/r/20170914225422.31034-1-mcg...@kernel.org
>
> > struct firmware_cache {
> > /* firmware_buf instance will be added into the below list */
> > @@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
> > if (ret <= 0) /* error or already assigned */
> > goto out;
> >
> > - ret = fw_get_filesystem_firmware(device, fw->priv);
> > + if (opt_flags & FW_OPT_PREFER_USER) {
> > + ret = fw_load_from_user_helper(fw, name, device, opt_flags,
> > timeout);
> > + if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
> > + dev_warn(device,
> > +"User helper firmware load for %s failed with
> > error %d\n",
> > +name, ret);
> > + dev_warn(device, "Falling back to direct firmware
> > load\n");
>
> As I had noted before, the usermode helper was really not well designed,
> as such extending further use of it is something we should shy away unless we
> determine its completely necessary.
>
> So what's wrong with this driver failing at direct access, which should be
> fast,
> and relying on a uevent to then work using the current fallback mechanisms?
>
> The commit log in no way documents any of the justifications for further
> extending use of the usermode helper.
Hi! See patch 6/6. It is needed to avoid direct access and wl1251 on
Nokia N900 needs to use userspace helper which prepares firmware data.
Direct access is just fallback when userspace helper is not available.
Without userspace helper on devices where wl1251 do not have own eeprom
memory, wl1251 cannot work.
I know that usermode helper is not well designed, but it is the best
option what we can do for wl1251.
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function
On Friday 10 November 2017 21:26:01 Luis R. Rodriguez wrote:
> On Fri, Nov 10, 2017 at 12:38:27AM +0100, Pali Rohár wrote:
> > This function works pretty much like request_firmware(), but it prefer
> > usermode helper. If usermode helper fails then it fallback to direct
> > access. Useful for dynamic or model specific firmware data.
> >
> > Signed-off-by: Pali Rohár
> > ---
> > drivers/base/firmware_class.c | 45
> > +++--
> > include/linux/firmware.h |9 +
> > 2 files changed, 52 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 4b57cf5..c3a9fe5 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st,
> > enum fw_status status)
> > #endif
> > #define FW_OPT_NO_WARN (1U << 3)
> > #define FW_OPT_NOCACHE (1U << 4)
> > +#ifdef CONFIG_FW_LOADER_USER_HELPER
> > +#define FW_OPT_PREFER_USER (1U << 5)
> > +#else
> > +#define FW_OPT_PREFER_USER 0
> > +#endif
>
> I've been cleaning these up these flags [0], which I'll shortly respin based
> on feedback, so this sort of stuff should be avoided at all costs.
>
> Regardless of this even if you *leave* the flag in place and a driver required
> this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
> calling fw_load_from_user_helper would just already return -ENOENT, as such it
> would in turn fallback to direct fs loading so the #ifef'ery seems to be not
> needed.
>
> [0] https://lkml.kernel.org/r/20170914225422.31034-1-mcg...@kernel.org
>
> > struct firmware_cache {
> > /* firmware_buf instance will be added into the below list */
> > @@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
> > if (ret <= 0) /* error or already assigned */
> > goto out;
> >
> > - ret = fw_get_filesystem_firmware(device, fw->priv);
> > + if (opt_flags & FW_OPT_PREFER_USER) {
> > + ret = fw_load_from_user_helper(fw, name, device, opt_flags,
> > timeout);
> > + if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
> > + dev_warn(device,
> > +"User helper firmware load for %s failed with
> > error %d\n",
> > +name, ret);
> > + dev_warn(device, "Falling back to direct firmware
> > load\n");
>
> As I had noted before, the usermode helper was really not well designed,
> as such extending further use of it is something we should shy away unless we
> determine its completely necessary.
>
> So what's wrong with this driver failing at direct access, which should be
> fast,
> and relying on a uevent to then work using the current fallback mechanisms?
>
> The commit log in no way documents any of the justifications for further
> extending use of the usermode helper.
Hi! See patch 6/6. It is needed to avoid direct access and wl1251 on
Nokia N900 needs to use userspace helper which prepares firmware data.
Direct access is just fallback when userspace helper is not available.
Without userspace helper on devices where wl1251 do not have own eeprom
memory, wl1251 cannot work.
I know that usermode helper is not well designed, but it is the best
option what we can do for wl1251.
--
Pali Rohár
pali.ro...@gmail.com
N900 kernel support
Hi! Yesterday I updated https://elinux.org/N900 page based on data which I found in linux git repository. So driver status should be up-to-date. I would like to ask, what is current status of the bluetooth driver? It is working on N900, or it has a problems? Also are the any plans for bringing front webcam to work? IIRC there was problem that it was not possible to describe in DT that back and front cameras. And what happened with ramp mode for camera autofocus? -- Pali Rohár pali.ro...@gmail.com
N900 kernel support
Hi! Yesterday I updated https://elinux.org/N900 page based on data which I found in linux git repository. So driver status should be up-to-date. I would like to ask, what is current status of the bluetooth driver? It is working on N900, or it has a problems? Also are the any plans for bringing front webcam to work? IIRC there was problem that it was not possible to describe in DT that back and front cameras. And what happened with ramp mode for camera autofocus? -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v2 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers
>
> -#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
> +/* possible return values:
> + * -ENODEV: Descriptor GUID missing from WMI bus
> + * -EPROBE_DEFER: probing for dell-wmi-descriptor not yet run
> + * 0: valid descriptor, successfully probed
> + * < 0: invalid descriptor, don't probe dependent devices
> + */
> +int dell_wmi_get_descriptor_valid(void);
>
> bool dell_wmi_get_interface_version(u32 *version);
> bool dell_wmi_get_size(u32 *size);
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 54321080a30d..39d2f4518483 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -655,9 +655,11 @@ static int dell_wmi_events_set_enabled(bool enable)
> static int dell_wmi_probe(struct wmi_device *wdev)
> {
> struct dell_wmi_priv *priv;
> + int ret;
>
> - if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
> - return -ENODEV;
> + ret = dell_wmi_get_descriptor_valid();
> + if (ret)
> + return ret;
>
> priv = devm_kzalloc(
> >dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
Looks good,
Reviewed-by: Pali Rohár <pali.ro...@gmail.com>
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH v2 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers
UID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
> +/* possible return values:
> + * -ENODEV: Descriptor GUID missing from WMI bus
> + * -EPROBE_DEFER: probing for dell-wmi-descriptor not yet run
> + * 0: valid descriptor, successfully probed
> + * < 0: invalid descriptor, don't probe dependent devices
> + */
> +int dell_wmi_get_descriptor_valid(void);
>
> bool dell_wmi_get_interface_version(u32 *version);
> bool dell_wmi_get_size(u32 *size);
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 54321080a30d..39d2f4518483 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -655,9 +655,11 @@ static int dell_wmi_events_set_enabled(bool enable)
> static int dell_wmi_probe(struct wmi_device *wdev)
> {
> struct dell_wmi_priv *priv;
> + int ret;
>
> - if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
> - return -ENODEV;
> + ret = dell_wmi_get_descriptor_valid();
> + if (ret)
> + return ret;
>
> priv = devm_kzalloc(
> >dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
Looks good,
Reviewed-by: Pali Rohár
--
Pali Rohár
pali.ro...@gmail.com
[PATCH v2 2/6] wl1251: Generate random MAC address only if driver does not have valid
Before this patch, driver generated random MAC address every time it was
initialized. After that random MAC address could be overwritten with fixed
one, if provided.
This patch changes order. First it tries to read fixed MAC address and if
it fails then driver generates random MAC address.
Signed-off-by: Pali Rohár <pali.ro...@gmail.com>
Acked-by: Pavel Machek <pa...@ucw.cz>
---
drivers/net/wireless/ti/wl1251/main.c | 27 ++-
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/ti/wl1251/main.c
b/drivers/net/wireless/ti/wl1251/main.c
index 8929bb3..9106c20 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1492,7 +1492,24 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
wl->hw->queues = 4;
if (wl->use_eeprom)
- wl1251_read_eeprom_mac(wl);
+ ret = wl1251_read_eeprom_mac(wl);
+ else
+ ret = -EINVAL;
+
+ if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
+ ret = -EINVAL;
+
+ if (ret < 0) {
+ /*
+* In case our MAC address is not correctly set,
+* we use a random but Nokia MAC.
+*/
+ static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
+ memcpy(wl->mac_addr, nokia_oui, 3);
+ get_random_bytes(wl->mac_addr + 3, 3);
+ wl1251_warning("MAC address in eeprom or nvs data is not
valid");
+ wl1251_warning("Setting random MAC address: %pM", wl->mac_addr);
+ }
ret = wl1251_register_hw(wl);
if (ret)
@@ -1513,7 +1530,6 @@ struct ieee80211_hw *wl1251_alloc_hw(void)
struct ieee80211_hw *hw;
struct wl1251 *wl;
int i;
- static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
hw = ieee80211_alloc_hw(sizeof(*wl), _ops);
if (!hw) {
@@ -1563,13 +1579,6 @@ struct ieee80211_hw *wl1251_alloc_hw(void)
INIT_WORK(>irq_work, wl1251_irq_work);
INIT_WORK(>tx_work, wl1251_tx_work);
- /*
-* In case our MAC address is not correctly set,
-* we use a random but Nokia MAC.
-*/
- memcpy(wl->mac_addr, nokia_oui, 3);
- get_random_bytes(wl->mac_addr + 3, 3);
-
wl->state = WL1251_STATE_OFF;
mutex_init(>mutex);
spin_lock_init(>wl_lock);
--
1.7.9.5
[PATCH v2 2/6] wl1251: Generate random MAC address only if driver does not have valid
Before this patch, driver generated random MAC address every time it was
initialized. After that random MAC address could be overwritten with fixed
one, if provided.
This patch changes order. First it tries to read fixed MAC address and if
it fails then driver generates random MAC address.
Signed-off-by: Pali Rohár
Acked-by: Pavel Machek
---
drivers/net/wireless/ti/wl1251/main.c | 27 ++-
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/ti/wl1251/main.c
b/drivers/net/wireless/ti/wl1251/main.c
index 8929bb3..9106c20 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1492,7 +1492,24 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
wl->hw->queues = 4;
if (wl->use_eeprom)
- wl1251_read_eeprom_mac(wl);
+ ret = wl1251_read_eeprom_mac(wl);
+ else
+ ret = -EINVAL;
+
+ if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
+ ret = -EINVAL;
+
+ if (ret < 0) {
+ /*
+* In case our MAC address is not correctly set,
+* we use a random but Nokia MAC.
+*/
+ static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
+ memcpy(wl->mac_addr, nokia_oui, 3);
+ get_random_bytes(wl->mac_addr + 3, 3);
+ wl1251_warning("MAC address in eeprom or nvs data is not
valid");
+ wl1251_warning("Setting random MAC address: %pM", wl->mac_addr);
+ }
ret = wl1251_register_hw(wl);
if (ret)
@@ -1513,7 +1530,6 @@ struct ieee80211_hw *wl1251_alloc_hw(void)
struct ieee80211_hw *hw;
struct wl1251 *wl;
int i;
- static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
hw = ieee80211_alloc_hw(sizeof(*wl), _ops);
if (!hw) {
@@ -1563,13 +1579,6 @@ struct ieee80211_hw *wl1251_alloc_hw(void)
INIT_WORK(>irq_work, wl1251_irq_work);
INIT_WORK(>tx_work, wl1251_tx_work);
- /*
-* In case our MAC address is not correctly set,
-* we use a random but Nokia MAC.
-*/
- memcpy(wl->mac_addr, nokia_oui, 3);
- get_random_bytes(wl->mac_addr + 3, 3);
-
wl->state = WL1251_STATE_OFF;
mutex_init(>mutex);
spin_lock_init(>wl_lock);
--
1.7.9.5
[PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function
This function works pretty much like request_firmware(), but it prefer
usermode helper. If usermode helper fails then it fallback to direct
access. Useful for dynamic or model specific firmware data.
Signed-off-by: Pali Rohár <pali.ro...@gmail.com>
---
drivers/base/firmware_class.c | 45 +++--
include/linux/firmware.h |9 +
2 files changed, 52 insertions(+), 2 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4b57cf5..c3a9fe5 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, enum
fw_status status)
#endif
#define FW_OPT_NO_WARN (1U << 3)
#define FW_OPT_NOCACHE (1U << 4)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_PREFER_USER (1U << 5)
+#else
+#define FW_OPT_PREFER_USER 0
+#endif
struct firmware_cache {
/* firmware_buf instance will be added into the below list */
@@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
if (ret <= 0) /* error or already assigned */
goto out;
- ret = fw_get_filesystem_firmware(device, fw->priv);
+ if (opt_flags & FW_OPT_PREFER_USER) {
+ ret = fw_load_from_user_helper(fw, name, device, opt_flags,
timeout);
+ if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
+ dev_warn(device,
+"User helper firmware load for %s failed with
error %d\n",
+name, ret);
+ dev_warn(device, "Falling back to direct firmware
load\n");
+ }
+ } else {
+ ret = -EINVAL;
+ }
+
+ if (ret)
+ ret = fw_get_filesystem_firmware(device, fw->priv);
if (ret) {
if (!(opt_flags & FW_OPT_NO_WARN))
dev_warn(device,
"Direct firmware load for %s failed with error
%d\n",
name, ret);
- if (opt_flags & FW_OPT_USERHELPER) {
+ if ((opt_flags & FW_OPT_USERHELPER) && !(opt_flags &
FW_OPT_PREFER_USER)) {
dev_warn(device, "Falling back to user helper\n");
ret = fw_load_from_user_helper(fw, name, device,
opt_flags);
@@ -1329,6 +1347,29 @@ int request_firmware_direct(const struct firmware
**firmware_p,
EXPORT_SYMBOL(request_firmware_into_buf);
/**
+ * request_firmware_prefer_user: - prefer usermode helper for loading firmware
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function works pretty much like request_firmware(), but it prefer
+ * usermode helper. If usermode helper fails then it fallback to direct access.
+ * Useful for dynamic or model specific firmware data.
+ **/
+int request_firmware_prefer_user(const struct firmware **firmware_p,
+ const char *name, struct device *device)
+{
+ int ret;
+
+ __module_get(THIS_MODULE);
+ ret = _request_firmware(firmware_p, name, device, NULL, 0,
+ FW_OPT_UEVENT | FW_OPT_PREFER_USER);
+ module_put(THIS_MODULE);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
+
+/**
* release_firmware: - release the resource associated with a firmware image
* @fw: firmware resource to release
**/
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index d450808..8584528 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -48,6 +48,8 @@ int request_firmware_nowait(
void (*cont)(const struct firmware *fw, void *context));
int request_firmware_direct(const struct firmware **fw, const char *name,
struct device *device);
+int request_firmware_prefer_user(const struct firmware **fw, const char *name,
+struct device *device);
int request_firmware_into_buf(const struct firmware **firmware_p,
const char *name, struct device *device, void *buf, size_t size);
@@ -78,6 +80,13 @@ static inline int request_firmware_direct(const struct
firmware **fw,
return -EINVAL;
}
+static inline int request_firmware_prefer_user(const struct firmware **fw,
+ const char *name,
+ struct device *device)
+{
+ return -EINVAL;
+}
+
static inline int request_firmware_into_buf(const struct firmware **firmware_p,
const char *name, struct device *device, void *buf, size_t size)
{
--
1.7.9.5
[PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function
This function works pretty much like request_firmware(), but it prefer
usermode helper. If usermode helper fails then it fallback to direct
access. Useful for dynamic or model specific firmware data.
Signed-off-by: Pali Rohár
---
drivers/base/firmware_class.c | 45 +++--
include/linux/firmware.h |9 +
2 files changed, 52 insertions(+), 2 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4b57cf5..c3a9fe5 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, enum
fw_status status)
#endif
#define FW_OPT_NO_WARN (1U << 3)
#define FW_OPT_NOCACHE (1U << 4)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_PREFER_USER (1U << 5)
+#else
+#define FW_OPT_PREFER_USER 0
+#endif
struct firmware_cache {
/* firmware_buf instance will be added into the below list */
@@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
if (ret <= 0) /* error or already assigned */
goto out;
- ret = fw_get_filesystem_firmware(device, fw->priv);
+ if (opt_flags & FW_OPT_PREFER_USER) {
+ ret = fw_load_from_user_helper(fw, name, device, opt_flags,
timeout);
+ if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
+ dev_warn(device,
+"User helper firmware load for %s failed with
error %d\n",
+name, ret);
+ dev_warn(device, "Falling back to direct firmware
load\n");
+ }
+ } else {
+ ret = -EINVAL;
+ }
+
+ if (ret)
+ ret = fw_get_filesystem_firmware(device, fw->priv);
if (ret) {
if (!(opt_flags & FW_OPT_NO_WARN))
dev_warn(device,
"Direct firmware load for %s failed with error
%d\n",
name, ret);
- if (opt_flags & FW_OPT_USERHELPER) {
+ if ((opt_flags & FW_OPT_USERHELPER) && !(opt_flags &
FW_OPT_PREFER_USER)) {
dev_warn(device, "Falling back to user helper\n");
ret = fw_load_from_user_helper(fw, name, device,
opt_flags);
@@ -1329,6 +1347,29 @@ int request_firmware_direct(const struct firmware
**firmware_p,
EXPORT_SYMBOL(request_firmware_into_buf);
/**
+ * request_firmware_prefer_user: - prefer usermode helper for loading firmware
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function works pretty much like request_firmware(), but it prefer
+ * usermode helper. If usermode helper fails then it fallback to direct access.
+ * Useful for dynamic or model specific firmware data.
+ **/
+int request_firmware_prefer_user(const struct firmware **firmware_p,
+ const char *name, struct device *device)
+{
+ int ret;
+
+ __module_get(THIS_MODULE);
+ ret = _request_firmware(firmware_p, name, device, NULL, 0,
+ FW_OPT_UEVENT | FW_OPT_PREFER_USER);
+ module_put(THIS_MODULE);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
+
+/**
* release_firmware: - release the resource associated with a firmware image
* @fw: firmware resource to release
**/
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index d450808..8584528 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -48,6 +48,8 @@ int request_firmware_nowait(
void (*cont)(const struct firmware *fw, void *context));
int request_firmware_direct(const struct firmware **fw, const char *name,
struct device *device);
+int request_firmware_prefer_user(const struct firmware **fw, const char *name,
+struct device *device);
int request_firmware_into_buf(const struct firmware **firmware_p,
const char *name, struct device *device, void *buf, size_t size);
@@ -78,6 +80,13 @@ static inline int request_firmware_direct(const struct
firmware **fw,
return -EINVAL;
}
+static inline int request_firmware_prefer_user(const struct firmware **fw,
+ const char *name,
+ struct device *device)
+{
+ return -EINVAL;
+}
+
static inline int request_firmware_into_buf(const struct firmware **firmware_p,
const char *name, struct device *device, void *buf, size_t size)
{
--
1.7.9.5
[PATCH v2 4/6] wl1251: Set generated MAC address back to NVS data
In case there is no valid MAC address kernel generates random one. This
patch propagate this generated MAC address back to NVS data which will be
uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
uses.
This should not change any functionality, but it is better to tell wl1251
correct mac address since beginning of chip usage.
Signed-off-by: Pali Rohár <pali.ro...@gmail.com>
---
drivers/net/wireless/ti/wl1251/main.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/net/wireless/ti/wl1251/main.c
b/drivers/net/wireless/ti/wl1251/main.c
index d497ba5..1f423be 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1481,6 +1481,21 @@ static int wl1251_read_nvs_mac(struct wl1251 *wl)
return 0;
}
+static int wl1251_write_nvs_mac(struct wl1251 *wl)
+{
+ int i, ret;
+
+ ret = wl1251_check_nvs_mac(wl);
+ if (ret)
+ return ret;
+
+ /* MAC is stored in reverse order */
+ for (i = 0; i < ETH_ALEN; i++)
+ wl->nvs[NVS_OFF_MAC_DATA + i] = wl->mac_addr[ETH_ALEN - i - 1];
+
+ return 0;
+}
+
static int wl1251_register_hw(struct wl1251 *wl)
{
int ret;
@@ -1546,6 +1561,8 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
memcpy(wl->mac_addr, nokia_oui, 3);
get_random_bytes(wl->mac_addr + 3, 3);
+ if (!wl->use_eeprom)
+ wl1251_write_nvs_mac(wl);
wl1251_warning("MAC address in eeprom or nvs data is not
valid");
wl1251_warning("Setting random MAC address: %pM", wl->mac_addr);
}
--
1.7.9.5
[PATCH v2 4/6] wl1251: Set generated MAC address back to NVS data
In case there is no valid MAC address kernel generates random one. This
patch propagate this generated MAC address back to NVS data which will be
uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
uses.
This should not change any functionality, but it is better to tell wl1251
correct mac address since beginning of chip usage.
Signed-off-by: Pali Rohár
---
drivers/net/wireless/ti/wl1251/main.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/net/wireless/ti/wl1251/main.c
b/drivers/net/wireless/ti/wl1251/main.c
index d497ba5..1f423be 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1481,6 +1481,21 @@ static int wl1251_read_nvs_mac(struct wl1251 *wl)
return 0;
}
+static int wl1251_write_nvs_mac(struct wl1251 *wl)
+{
+ int i, ret;
+
+ ret = wl1251_check_nvs_mac(wl);
+ if (ret)
+ return ret;
+
+ /* MAC is stored in reverse order */
+ for (i = 0; i < ETH_ALEN; i++)
+ wl->nvs[NVS_OFF_MAC_DATA + i] = wl->mac_addr[ETH_ALEN - i - 1];
+
+ return 0;
+}
+
static int wl1251_register_hw(struct wl1251 *wl)
{
int ret;
@@ -1546,6 +1561,8 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
memcpy(wl->mac_addr, nokia_oui, 3);
get_random_bytes(wl->mac_addr + 3, 3);
+ if (!wl->use_eeprom)
+ wl1251_write_nvs_mac(wl);
wl1251_warning("MAC address in eeprom or nvs data is not
valid");
wl1251_warning("Setting random MAC address: %pM", wl->mac_addr);
}
--
1.7.9.5
[PATCH v2 1/6] wl1251: Update wl->nvs_len after wl->nvs is valid
If kmemdup fails, then wl->nvs_len will contain invalid non-zero size.
Signed-off-by: Pali Rohár <pali.ro...@gmail.com>
Acked-by: Pavel Machek <pa...@ucw.cz>
---
drivers/net/wireless/ti/wl1251/main.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ti/wl1251/main.c
b/drivers/net/wireless/ti/wl1251/main.c
index 9915d83..8929bb3 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -122,8 +122,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
goto out;
}
- wl->nvs_len = fw->size;
- wl->nvs = kmemdup(fw->data, wl->nvs_len, GFP_KERNEL);
+ wl->nvs = kmemdup(fw->data, fw->size, GFP_KERNEL);
if (!wl->nvs) {
wl1251_error("could not allocate memory for the nvs file");
@@ -131,6 +130,8 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
goto out;
}
+ wl->nvs_len = fw->size;
+
ret = 0;
out:
--
1.7.9.5
[PATCH v2 1/6] wl1251: Update wl->nvs_len after wl->nvs is valid
If kmemdup fails, then wl->nvs_len will contain invalid non-zero size.
Signed-off-by: Pali Rohár
Acked-by: Pavel Machek
---
drivers/net/wireless/ti/wl1251/main.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ti/wl1251/main.c
b/drivers/net/wireless/ti/wl1251/main.c
index 9915d83..8929bb3 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -122,8 +122,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
goto out;
}
- wl->nvs_len = fw->size;
- wl->nvs = kmemdup(fw->data, wl->nvs_len, GFP_KERNEL);
+ wl->nvs = kmemdup(fw->data, fw->size, GFP_KERNEL);
if (!wl->nvs) {
wl1251_error("could not allocate memory for the nvs file");
@@ -131,6 +130,8 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
goto out;
}
+ wl->nvs_len = fw->size;
+
ret = 0;
out:
--
1.7.9.5
[PATCH v2 3/6] wl1251: Parse and use MAC address from supplied NVS data
This patch implements parsing MAC address from NVS data which are sent to
wl1251 chip. Calibration NVS data could contain valid MAC address and it
will be used instead of randomly generated one.
This patch also moves code for requesting NVS data from userspace to driver
initialization code to make sure that NVS data will be there at time when
permanent MAC address is needed.
Calibration NVS data for wl1251 are device specific. Every device with
wl1251 chip should have been calibrated in factory and needs to provide own
calibration data.
Default example file wl1251-nvs.bin, found in linux-firmware repository,
contains MAC address 00:00:20:07:03:09. So this MAC address is marked as
invalid as it is not real device specific address, just example one.
Format of calibration NVS data can be found at:
http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
Signed-off-by: Pali Rohár <pali.ro...@gmail.com>
---
drivers/net/wireless/ti/wl1251/main.c | 55 -
1 file changed, 47 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ti/wl1251/main.c
b/drivers/net/wireless/ti/wl1251/main.c
index 9106c20..d497ba5 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -203,13 +203,6 @@ static int wl1251_chip_wakeup(struct wl1251 *wl)
goto out;
}
- if (wl->nvs == NULL && !wl->use_eeprom) {
- /* No NVS from netlink, try to get it from the filesystem */
- ret = wl1251_fetch_nvs(wl);
- if (ret < 0)
- goto out;
- }
-
out:
return ret;
}
@@ -1448,6 +1441,46 @@ static int wl1251_read_eeprom_mac(struct wl1251 *wl)
return 0;
}
+#define NVS_OFF_MAC_LEN 0x19
+#define NVS_OFF_MAC_ADDR_LO 0x1a
+#define NVS_OFF_MAC_ADDR_HI 0x1b
+#define NVS_OFF_MAC_DATA 0x1c
+
+static int wl1251_check_nvs_mac(struct wl1251 *wl)
+{
+ if (wl->nvs_len < 0x24)
+ return -ENODATA;
+
+ /* length is 2 and data address is 0x546c (ANDed with 0xfffe) */
+ if (wl->nvs[NVS_OFF_MAC_LEN] != 2 ||
+ wl->nvs[NVS_OFF_MAC_ADDR_LO] != 0x6d ||
+ wl->nvs[NVS_OFF_MAC_ADDR_HI] != 0x54)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int wl1251_read_nvs_mac(struct wl1251 *wl)
+{
+ u8 mac[ETH_ALEN];
+ int i, ret;
+
+ ret = wl1251_check_nvs_mac(wl);
+ if (ret)
+ return ret;
+
+ /* MAC is stored in reverse order */
+ for (i = 0; i < ETH_ALEN; i++)
+ mac[i] = wl->nvs[NVS_OFF_MAC_DATA + ETH_ALEN - i - 1];
+
+ /* 00:00:20:07:03:09 is in example file wl1251-nvs.bin, so invalid */
+ if (ether_addr_equal_unaligned(mac, "\x00\x00\x20\x07\x03\x09"))
+ return -EINVAL;
+
+ memcpy(wl->mac_addr, mac, ETH_ALEN);
+ return 0;
+}
+
static int wl1251_register_hw(struct wl1251 *wl)
{
int ret;
@@ -1491,10 +1524,16 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
wl->hw->queues = 4;
+ if (wl->nvs == NULL && !wl->use_eeprom) {
+ ret = wl1251_fetch_nvs(wl);
+ if (ret < 0)
+ goto out;
+ }
+
if (wl->use_eeprom)
ret = wl1251_read_eeprom_mac(wl);
else
- ret = -EINVAL;
+ ret = wl1251_read_nvs_mac(wl);
if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
ret = -EINVAL;
--
1.7.9.5
[PATCH v2 6/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
NVS calibration data for wl1251 are model specific. Every one device with
wl1251 chip has different and calibrated in factory.
Not all wl1251 chips have own EEPROM where are calibration data stored. And
in that case there is no "standard" place. Every device has stored them on
different place (some in rootfs file, some in dedicated nand partition,
some in another proprietary structure).
Kernel wl1251 driver cannot support every one different storage decided by
device manufacture so it will use request_firmware_prefer_user() call for
loading NVS calibration data and userspace helper will be responsible to
prepare correct data.
In case userspace helper fails request_firmware_prefer_user() still try to
load data file directly from VFS as fallback mechanism.
On Nokia N900 device, which has wl1251 chip, NVS calibration data are
stored in CAL nand partition. CAL is proprietary Nokia key/value format for
nand devices.
With this patch it is finally possible to load correct model specific NVS
calibration data for Nokia N900.
Userspace tool for reading NVS calibration data on Nokia N900 is available
in git repository at: https://github.com/community-ssu/wl1251-cal
Signed-off-by: Pali Rohár <pali.ro...@gmail.com>
---
drivers/net/wireless/ti/wl1251/Kconfig |1 +
drivers/net/wireless/ti/wl1251/main.c |2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ti/wl1251/Kconfig
b/drivers/net/wireless/ti/wl1251/Kconfig
index 7142ccf..affe154 100644
--- a/drivers/net/wireless/ti/wl1251/Kconfig
+++ b/drivers/net/wireless/ti/wl1251/Kconfig
@@ -2,6 +2,7 @@ config WL1251
tristate "TI wl1251 driver support"
depends on MAC80211
select FW_LOADER
+ select FW_LOADER_USER_HELPER
select CRC7
---help---
This will enable TI wl1251 driver support. The drivers make
diff --git a/drivers/net/wireless/ti/wl1251/main.c
b/drivers/net/wireless/ti/wl1251/main.c
index 1f423be..e9d232c 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -108,7 +108,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
struct device *dev = wiphy_dev(wl->hw->wiphy);
int ret;
- ret = request_firmware(, WL1251_NVS_NAME, dev);
+ ret = request_firmware_prefer_user(, WL1251_NVS_NAME, dev);
if (ret < 0) {
wl1251_error("could not get nvs file: %d", ret);
--
1.7.9.5
[PATCH v2 3/6] wl1251: Parse and use MAC address from supplied NVS data
This patch implements parsing MAC address from NVS data which are sent to
wl1251 chip. Calibration NVS data could contain valid MAC address and it
will be used instead of randomly generated one.
This patch also moves code for requesting NVS data from userspace to driver
initialization code to make sure that NVS data will be there at time when
permanent MAC address is needed.
Calibration NVS data for wl1251 are device specific. Every device with
wl1251 chip should have been calibrated in factory and needs to provide own
calibration data.
Default example file wl1251-nvs.bin, found in linux-firmware repository,
contains MAC address 00:00:20:07:03:09. So this MAC address is marked as
invalid as it is not real device specific address, just example one.
Format of calibration NVS data can be found at:
http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
Signed-off-by: Pali Rohár
---
drivers/net/wireless/ti/wl1251/main.c | 55 -
1 file changed, 47 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ti/wl1251/main.c
b/drivers/net/wireless/ti/wl1251/main.c
index 9106c20..d497ba5 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -203,13 +203,6 @@ static int wl1251_chip_wakeup(struct wl1251 *wl)
goto out;
}
- if (wl->nvs == NULL && !wl->use_eeprom) {
- /* No NVS from netlink, try to get it from the filesystem */
- ret = wl1251_fetch_nvs(wl);
- if (ret < 0)
- goto out;
- }
-
out:
return ret;
}
@@ -1448,6 +1441,46 @@ static int wl1251_read_eeprom_mac(struct wl1251 *wl)
return 0;
}
+#define NVS_OFF_MAC_LEN 0x19
+#define NVS_OFF_MAC_ADDR_LO 0x1a
+#define NVS_OFF_MAC_ADDR_HI 0x1b
+#define NVS_OFF_MAC_DATA 0x1c
+
+static int wl1251_check_nvs_mac(struct wl1251 *wl)
+{
+ if (wl->nvs_len < 0x24)
+ return -ENODATA;
+
+ /* length is 2 and data address is 0x546c (ANDed with 0xfffe) */
+ if (wl->nvs[NVS_OFF_MAC_LEN] != 2 ||
+ wl->nvs[NVS_OFF_MAC_ADDR_LO] != 0x6d ||
+ wl->nvs[NVS_OFF_MAC_ADDR_HI] != 0x54)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int wl1251_read_nvs_mac(struct wl1251 *wl)
+{
+ u8 mac[ETH_ALEN];
+ int i, ret;
+
+ ret = wl1251_check_nvs_mac(wl);
+ if (ret)
+ return ret;
+
+ /* MAC is stored in reverse order */
+ for (i = 0; i < ETH_ALEN; i++)
+ mac[i] = wl->nvs[NVS_OFF_MAC_DATA + ETH_ALEN - i - 1];
+
+ /* 00:00:20:07:03:09 is in example file wl1251-nvs.bin, so invalid */
+ if (ether_addr_equal_unaligned(mac, "\x00\x00\x20\x07\x03\x09"))
+ return -EINVAL;
+
+ memcpy(wl->mac_addr, mac, ETH_ALEN);
+ return 0;
+}
+
static int wl1251_register_hw(struct wl1251 *wl)
{
int ret;
@@ -1491,10 +1524,16 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
wl->hw->queues = 4;
+ if (wl->nvs == NULL && !wl->use_eeprom) {
+ ret = wl1251_fetch_nvs(wl);
+ if (ret < 0)
+ goto out;
+ }
+
if (wl->use_eeprom)
ret = wl1251_read_eeprom_mac(wl);
else
- ret = -EINVAL;
+ ret = wl1251_read_nvs_mac(wl);
if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
ret = -EINVAL;
--
1.7.9.5
[PATCH v2 6/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
NVS calibration data for wl1251 are model specific. Every one device with
wl1251 chip has different and calibrated in factory.
Not all wl1251 chips have own EEPROM where are calibration data stored. And
in that case there is no "standard" place. Every device has stored them on
different place (some in rootfs file, some in dedicated nand partition,
some in another proprietary structure).
Kernel wl1251 driver cannot support every one different storage decided by
device manufacture so it will use request_firmware_prefer_user() call for
loading NVS calibration data and userspace helper will be responsible to
prepare correct data.
In case userspace helper fails request_firmware_prefer_user() still try to
load data file directly from VFS as fallback mechanism.
On Nokia N900 device, which has wl1251 chip, NVS calibration data are
stored in CAL nand partition. CAL is proprietary Nokia key/value format for
nand devices.
With this patch it is finally possible to load correct model specific NVS
calibration data for Nokia N900.
Userspace tool for reading NVS calibration data on Nokia N900 is available
in git repository at: https://github.com/community-ssu/wl1251-cal
Signed-off-by: Pali Rohár
---
drivers/net/wireless/ti/wl1251/Kconfig |1 +
drivers/net/wireless/ti/wl1251/main.c |2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ti/wl1251/Kconfig
b/drivers/net/wireless/ti/wl1251/Kconfig
index 7142ccf..affe154 100644
--- a/drivers/net/wireless/ti/wl1251/Kconfig
+++ b/drivers/net/wireless/ti/wl1251/Kconfig
@@ -2,6 +2,7 @@ config WL1251
tristate "TI wl1251 driver support"
depends on MAC80211
select FW_LOADER
+ select FW_LOADER_USER_HELPER
select CRC7
---help---
This will enable TI wl1251 driver support. The drivers make
diff --git a/drivers/net/wireless/ti/wl1251/main.c
b/drivers/net/wireless/ti/wl1251/main.c
index 1f423be..e9d232c 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -108,7 +108,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
struct device *dev = wiphy_dev(wl->hw->wiphy);
int ret;
- ret = request_firmware(, WL1251_NVS_NAME, dev);
+ ret = request_firmware_prefer_user(, WL1251_NVS_NAME, dev);
if (ret < 0) {
wl1251_error("could not get nvs file: %d", ret);
--
1.7.9.5
[PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900
This patch series fix processing MAC address for wl1251 chip found in Nokia N900. Changes since v1: * Added Acked-by for Pavel Machek * Fixed grammar * Magic numbers for NVS offsets are replaced by defines * Check for validity of mac address NVS data is moved into function * Changed order of patches as Pavel requested Pali Rohár (6): wl1251: Update wl->nvs_len after wl->nvs is valid wl1251: Generate random MAC address only if driver does not have valid wl1251: Parse and use MAC address from supplied NVS data wl1251: Set generated MAC address back to NVS data firmware: Add request_firmware_prefer_user() function wl1251: Use request_firmware_prefer_user() for loading NVS calibration data drivers/base/firmware_class.c | 45 +- drivers/net/wireless/ti/wl1251/Kconfig |1 + drivers/net/wireless/ti/wl1251/main.c | 104 ++-- include/linux/firmware.h |9 +++ 4 files changed, 138 insertions(+), 21 deletions(-) -- 1.7.9.5
[PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900
This patch series fix processing MAC address for wl1251 chip found in Nokia N900. Changes since v1: * Added Acked-by for Pavel Machek * Fixed grammar * Magic numbers for NVS offsets are replaced by defines * Check for validity of mac address NVS data is moved into function * Changed order of patches as Pavel requested Pali Rohár (6): wl1251: Update wl->nvs_len after wl->nvs is valid wl1251: Generate random MAC address only if driver does not have valid wl1251: Parse and use MAC address from supplied NVS data wl1251: Set generated MAC address back to NVS data firmware: Add request_firmware_prefer_user() function wl1251: Use request_firmware_prefer_user() for loading NVS calibration data drivers/base/firmware_class.c | 45 +- drivers/net/wireless/ti/wl1251/Kconfig |1 + drivers/net/wireless/ti/wl1251/main.c | 104 ++-- include/linux/firmware.h |9 +++ 4 files changed, 138 insertions(+), 21 deletions(-) -- 1.7.9.5
Re: [PATCH] Add support for bq27521 battery monitor
On Thursday 09 November 2017 22:45:40 Pavel Machek wrote:
> On Thu 2017-11-09 22:29:45, Pali Rohár wrote:
> > On Thursday 09 November 2017 22:06:15 Pavel Machek wrote:
> > > diff --git a/drivers/power/supply/bq27xxx_battery.c
> > > b/drivers/power/supply/bq27xxx_battery.c
> > > index ed44439..ee2851a 100644
> > > --- a/drivers/power/supply/bq27xxx_battery.c
> > > +++ b/drivers/power/supply/bq27xxx_battery.c
> > > @@ -381,6 +381,30 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
> > > [BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> > > BQ27XXX_DM_REG_ROWS,
> > > },
> > > + [BQ27521] = { /* FIXME */
> > > + [BQ27XXX_REG_CTRL] = 0x02,
> > > + [BQ27XXX_REG_TEMP] = 0x0a,
> > > + [BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_VOLT] = 0x0c,
> > > + [BQ27XXX_REG_AI] = 0x0e,
> > > + [BQ27XXX_REG_FLAGS] = 0x08,
> > > + [BQ27XXX_REG_TTE] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_TTF] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_TTES] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_TTECP] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_NAC] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_FCC] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_CYCT] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_AE] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_SOC] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> > > + [BQ27XXX_DM_CTRL] = INVALID_REG_ADDR,
> > > + [BQ27XXX_DM_CLASS] = INVALID_REG_ADDR,
> > > + [BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR,
> > > + [BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
> > > + [BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
> > > + },
> > > [BQ27530] = {
> > > [BQ27XXX_REG_CTRL] = 0x00,
> > > [BQ27XXX_REG_TEMP] = 0x06,
> >
> > Hi! IIRC those registers are valid only for sn27521 chip in revision 14
> > (or new). For older revision is used different register map. And
> > detection of chip revision is somehow possible via some registers.
>
> Yes, I know.. I even had this, but as I can't test it, and early N950
> samples should be very very rare, I'd prefer the patch as is.
>
> @@ -1905,6 +1940,23 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info
> *di)
>
> dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION);
>
> + if (di->chip == BQ27521) {
> + int val;
> + val = di->bus.read(di, 0x32, false);
> + if (val == 0x2100) {
> + printk("rev. 13 chip detected; add support\n");
> + /* https://elinux.org/N950 has details */
> + return PTR_ERR(ENODEV);
> + }
> +
> + val = di->bus.read(di, 0x34, false);
> + if ((val & 0xff00) != 0x2100) {
> + printk("rev. 14 chip not detected?!\n");
> + return PTR_ERR(EINVAL);
> + }
> + printk("verified chip rev. 14\n");
> + }
> +
> bq27xxx_battery_settings(di);
> bq27xxx_battery_update(di);
This change could be useful, specially if somebody got device with
sn27521 chip which is not in revision 14 (no idea if in world are those
chips used) -- at least would get message that current driver does not
support it.
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH] Add support for bq27521 battery monitor
On Thursday 09 November 2017 22:45:40 Pavel Machek wrote:
> On Thu 2017-11-09 22:29:45, Pali Rohár wrote:
> > On Thursday 09 November 2017 22:06:15 Pavel Machek wrote:
> > > diff --git a/drivers/power/supply/bq27xxx_battery.c
> > > b/drivers/power/supply/bq27xxx_battery.c
> > > index ed44439..ee2851a 100644
> > > --- a/drivers/power/supply/bq27xxx_battery.c
> > > +++ b/drivers/power/supply/bq27xxx_battery.c
> > > @@ -381,6 +381,30 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
> > > [BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> > > BQ27XXX_DM_REG_ROWS,
> > > },
> > > + [BQ27521] = { /* FIXME */
> > > + [BQ27XXX_REG_CTRL] = 0x02,
> > > + [BQ27XXX_REG_TEMP] = 0x0a,
> > > + [BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_VOLT] = 0x0c,
> > > + [BQ27XXX_REG_AI] = 0x0e,
> > > + [BQ27XXX_REG_FLAGS] = 0x08,
> > > + [BQ27XXX_REG_TTE] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_TTF] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_TTES] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_TTECP] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_NAC] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_FCC] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_CYCT] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_AE] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_SOC] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
> > > + [BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> > > + [BQ27XXX_DM_CTRL] = INVALID_REG_ADDR,
> > > + [BQ27XXX_DM_CLASS] = INVALID_REG_ADDR,
> > > + [BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR,
> > > + [BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
> > > + [BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
> > > + },
> > > [BQ27530] = {
> > > [BQ27XXX_REG_CTRL] = 0x00,
> > > [BQ27XXX_REG_TEMP] = 0x06,
> >
> > Hi! IIRC those registers are valid only for sn27521 chip in revision 14
> > (or new). For older revision is used different register map. And
> > detection of chip revision is somehow possible via some registers.
>
> Yes, I know.. I even had this, but as I can't test it, and early N950
> samples should be very very rare, I'd prefer the patch as is.
>
> @@ -1905,6 +1940,23 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info
> *di)
>
> dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION);
>
> + if (di->chip == BQ27521) {
> + int val;
> + val = di->bus.read(di, 0x32, false);
> + if (val == 0x2100) {
> + printk("rev. 13 chip detected; add support\n");
> + /* https://elinux.org/N950 has details */
> + return PTR_ERR(ENODEV);
> + }
> +
> + val = di->bus.read(di, 0x34, false);
> + if ((val & 0xff00) != 0x2100) {
> + printk("rev. 14 chip not detected?!\n");
> + return PTR_ERR(EINVAL);
> + }
> + printk("verified chip rev. 14\n");
> + }
> +
> bq27xxx_battery_settings(di);
> bq27xxx_battery_update(di);
This change could be useful, specially if somebody got device with
sn27521 chip which is not in revision 14 (no idea if in world are those
chips used) -- at least would get message that current driver does not
support it.
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH] Add support for bq27521 battery monitor
On Thursday 09 November 2017 22:06:15 Pavel Machek wrote:
> diff --git a/drivers/power/supply/bq27xxx_battery.c
> b/drivers/power/supply/bq27xxx_battery.c
> index ed44439..ee2851a 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -381,6 +381,30 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
> [BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> BQ27XXX_DM_REG_ROWS,
> },
> + [BQ27521] = { /* FIXME */
> + [BQ27XXX_REG_CTRL] = 0x02,
> + [BQ27XXX_REG_TEMP] = 0x0a,
> + [BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_VOLT] = 0x0c,
> + [BQ27XXX_REG_AI] = 0x0e,
> + [BQ27XXX_REG_FLAGS] = 0x08,
> + [BQ27XXX_REG_TTE] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_TTF] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_TTES] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_TTECP] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_NAC] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_FCC] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_CYCT] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_AE] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_SOC] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> + [BQ27XXX_DM_CTRL] = INVALID_REG_ADDR,
> + [BQ27XXX_DM_CLASS] = INVALID_REG_ADDR,
> + [BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR,
> + [BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
> + [BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
> + },
> [BQ27530] = {
> [BQ27XXX_REG_CTRL] = 0x00,
> [BQ27XXX_REG_TEMP] = 0x06,
Hi! IIRC those registers are valid only for sn27521 chip in revision 14
(or new). For older revision is used different register map. And
detection of chip revision is somehow possible via some registers.
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH] Add support for bq27521 battery monitor
On Thursday 09 November 2017 22:06:15 Pavel Machek wrote:
> diff --git a/drivers/power/supply/bq27xxx_battery.c
> b/drivers/power/supply/bq27xxx_battery.c
> index ed44439..ee2851a 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -381,6 +381,30 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
> [BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> BQ27XXX_DM_REG_ROWS,
> },
> + [BQ27521] = { /* FIXME */
> + [BQ27XXX_REG_CTRL] = 0x02,
> + [BQ27XXX_REG_TEMP] = 0x0a,
> + [BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_VOLT] = 0x0c,
> + [BQ27XXX_REG_AI] = 0x0e,
> + [BQ27XXX_REG_FLAGS] = 0x08,
> + [BQ27XXX_REG_TTE] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_TTF] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_TTES] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_TTECP] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_NAC] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_FCC] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_CYCT] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_AE] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_SOC] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
> + [BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> + [BQ27XXX_DM_CTRL] = INVALID_REG_ADDR,
> + [BQ27XXX_DM_CLASS] = INVALID_REG_ADDR,
> + [BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR,
> + [BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
> + [BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
> + },
> [BQ27530] = {
> [BQ27XXX_REG_CTRL] = 0x00,
> [BQ27XXX_REG_TEMP] = 0x06,
Hi! IIRC those registers are valid only for sn27521 chip in revision 14
(or new). For older revision is used different register map. And
detection of chip revision is somehow possible via some registers.
--
Pali Rohár
pali.ro...@gmail.com
Re: Linux & FAT32 label
On Sunday 05 November 2017 14:06:08 Pali Rohár wrote: > On Wednesday 11 October 2017 23:24:35 Pali Rohár wrote: > > On Wednesday 04 October 2017 17:33:32 Pali Rohár wrote: > > > Hi! There is a big inconsistency in Linux tools which read or write > > > FAT32 label in filesystem images. The most common used are tools: > > > blkid (from util-linux project), fatlabel (previously known as > > > dosfslabel; from dosfstools project) and mlabel (from mtools project). > > > > > > FAT32 is itself a big mess from Microsoft hell and even FAT32 > > > implementation in Microsoft Windows systems is not compliant to the > > > released FAT32 documentation from Microsoft. > > > > > > In past months I observed that Linux FAT32 tools has its own way how > > > they interpret FAT32 label (known as volume id) and because every GUI > > > application uses one of those low-level command line tool, it is a big > > > mess if one application say that FAT32 label is A and another that it is > > > B. And then Windows XP say, it is C. > > > > > > I would like to open discussion if it would be possible to change > > > behavior how blkid (from util-linux project) and fatlabel (from > > > dosfstool project) handle FAT32 label. Ideally to report exactly same > > > output. > > > > > > Basic information about FAT32 label: > > > > > > 1) It is stored in two locations: boot sector and root directory as > > >file name. > > > > > > 2) In both location format is 11 bytes, padded with spaces (not nulls). > > > > > > 3) Empty label in boot sector is stored as "NO NAME" and not as > > >empty string. > > > > > > 4) Empty label in root directory is stored either as name which starts > > >with byte 0xE5, or is not stored in root directory at all. > > > > > > 5) If label contains leading byte 0xE5, then in root directory is stored > > >as byte 0x05. > > > > > > 6) Label string is stored according to current DOS code page. Therefore > > >label string needs to be converted to bytes. > > > > > > 7) Label string cannot contain control characters and characters from > > >the set ? / \ | . , ; : + = [ ] < > " plus lower case characters > > >are stored as their upper case variant (not only ASCII). > > > > > > (Please correct me if I'm wrong in some of those points) > > > > > > Plus Microsoft Windows systems fully ignores label stored in boot > > > sector. Seems they do not read it nor they do not update it on changes. > > > > > > Looks like that mlabel (from mtools) applies all above rules and uses > > > DOS code page 850 by default (can be changed in config file). > > > > > > blkid and fatlabel process special cases from 1) to 5) differently and > > > they operates on raw bytes, not strings (in DOS code page). > > > > > > mlabel reads label from the root directory (missing entry is interpreted > > > as no label; there is no fallback to boot sector), but "set" operation > > > modify label in both location boot sector + root directory. Basically it > > > is near to Windows implementation. And reason why Gparted GUI > > > application uses mlabel and not fatlabel. > > > > > > As Linux does not have "current DOS code page" and argv arguments are > > > not (Unicode) strings, but arbitrary bytes, I understand that for point > > > 6) it is easier to operates not on FAT strings (in current code page), > > > but rather on bytes. Which also would be same on all machines with any > > > configuration. > > > > > > But would it be possible to decide and unify handling of point 2), 3), > > > 4), 5)? Ideally with combination how to handle situation when different > > > label is stored in boot sector and root directory. > > > > > > As Windows does not use label in boot sector, it is very common > > > situation that label in boot sector differs from the root directory. > > > > > > The best would be see in all cases same label from blkid, fatlabel and > > > mlabel. Ideally same as Windows machines -- but due to DOS code page, > > > this is possible only for ASCII subset of the 8bit encoding. IIRC most > > > (or all?) DOS code page has same characters in printable ASCII range. > > > > > > It is really bad situation if I open disk in Gparted which show me label > >
Re: Linux & FAT32 label
On Sunday 05 November 2017 14:06:08 Pali Rohár wrote: > On Wednesday 11 October 2017 23:24:35 Pali Rohár wrote: > > On Wednesday 04 October 2017 17:33:32 Pali Rohár wrote: > > > Hi! There is a big inconsistency in Linux tools which read or write > > > FAT32 label in filesystem images. The most common used are tools: > > > blkid (from util-linux project), fatlabel (previously known as > > > dosfslabel; from dosfstools project) and mlabel (from mtools project). > > > > > > FAT32 is itself a big mess from Microsoft hell and even FAT32 > > > implementation in Microsoft Windows systems is not compliant to the > > > released FAT32 documentation from Microsoft. > > > > > > In past months I observed that Linux FAT32 tools has its own way how > > > they interpret FAT32 label (known as volume id) and because every GUI > > > application uses one of those low-level command line tool, it is a big > > > mess if one application say that FAT32 label is A and another that it is > > > B. And then Windows XP say, it is C. > > > > > > I would like to open discussion if it would be possible to change > > > behavior how blkid (from util-linux project) and fatlabel (from > > > dosfstool project) handle FAT32 label. Ideally to report exactly same > > > output. > > > > > > Basic information about FAT32 label: > > > > > > 1) It is stored in two locations: boot sector and root directory as > > >file name. > > > > > > 2) In both location format is 11 bytes, padded with spaces (not nulls). > > > > > > 3) Empty label in boot sector is stored as "NO NAME" and not as > > >empty string. > > > > > > 4) Empty label in root directory is stored either as name which starts > > >with byte 0xE5, or is not stored in root directory at all. > > > > > > 5) If label contains leading byte 0xE5, then in root directory is stored > > >as byte 0x05. > > > > > > 6) Label string is stored according to current DOS code page. Therefore > > >label string needs to be converted to bytes. > > > > > > 7) Label string cannot contain control characters and characters from > > >the set ? / \ | . , ; : + = [ ] < > " plus lower case characters > > >are stored as their upper case variant (not only ASCII). > > > > > > (Please correct me if I'm wrong in some of those points) > > > > > > Plus Microsoft Windows systems fully ignores label stored in boot > > > sector. Seems they do not read it nor they do not update it on changes. > > > > > > Looks like that mlabel (from mtools) applies all above rules and uses > > > DOS code page 850 by default (can be changed in config file). > > > > > > blkid and fatlabel process special cases from 1) to 5) differently and > > > they operates on raw bytes, not strings (in DOS code page). > > > > > > mlabel reads label from the root directory (missing entry is interpreted > > > as no label; there is no fallback to boot sector), but "set" operation > > > modify label in both location boot sector + root directory. Basically it > > > is near to Windows implementation. And reason why Gparted GUI > > > application uses mlabel and not fatlabel. > > > > > > As Linux does not have "current DOS code page" and argv arguments are > > > not (Unicode) strings, but arbitrary bytes, I understand that for point > > > 6) it is easier to operates not on FAT strings (in current code page), > > > but rather on bytes. Which also would be same on all machines with any > > > configuration. > > > > > > But would it be possible to decide and unify handling of point 2), 3), > > > 4), 5)? Ideally with combination how to handle situation when different > > > label is stored in boot sector and root directory. > > > > > > As Windows does not use label in boot sector, it is very common > > > situation that label in boot sector differs from the root directory. > > > > > > The best would be see in all cases same label from blkid, fatlabel and > > > mlabel. Ideally same as Windows machines -- but due to DOS code page, > > > this is possible only for ASCII subset of the 8bit encoding. IIRC most > > > (or all?) DOS code page has same characters in printable ASCII range. > > > > > > It is really bad situation if I open disk in Gparted which show me label > >
Re: Linux & FAT32 label
On Thursday 09 November 2017 11:21:25 Theodore Ts'o wrote: > On Thu, Nov 09, 2017 at 10:01:45AM +0100, Pali Rohár wrote: > > > > You would have stored LABEL42 in boot sector and no label in root > > > > directory. Windows handle this situation as there is no label. > > > > > > But why should we *care*? > > > > FAT is Microsoft's filesystem and the only usage of it on Linux is due > > to interoperability with different non-Linux systems. So here we should > > implement FAT in the similar/same way as other systems. It does not make > > sense to implement it differently and specially in non-compatible way. > > Because it lost reason what is primary usage of the FAT on Linux. > > The primary usage of FAT on Linux is for data interchange, primarily > with USB sticks. I'm still failing to see how it might "spoil some > vast eternal plan"[1] if a USB stick which Windows would show as > having no label, blkid/udev showed as having the label "LABEL42"? > > [1] to quote the song "If I were a rich man" > > I'm just trying to understand why this specific detail of bit-for-bit > compatibility is so important. Then you can ask question: Why we are serving LABEL information to users? Some users really use LABEL as identifier for themself, what is either stored on filesystem (in this case USB stick). And if somebody for some reason want to remove label (just because it does not contain data which label describe anymore), then user probably really want to use this information. LABEL is human readable identifier of particular filesystem on disk. If one system show one label and another system show another different label for same disk, then such information is useless. -- Pali Rohár pali.ro...@gmail.com
Re: Linux & FAT32 label
On Thursday 09 November 2017 11:21:25 Theodore Ts'o wrote: > On Thu, Nov 09, 2017 at 10:01:45AM +0100, Pali Rohár wrote: > > > > You would have stored LABEL42 in boot sector and no label in root > > > > directory. Windows handle this situation as there is no label. > > > > > > But why should we *care*? > > > > FAT is Microsoft's filesystem and the only usage of it on Linux is due > > to interoperability with different non-Linux systems. So here we should > > implement FAT in the similar/same way as other systems. It does not make > > sense to implement it differently and specially in non-compatible way. > > Because it lost reason what is primary usage of the FAT on Linux. > > The primary usage of FAT on Linux is for data interchange, primarily > with USB sticks. I'm still failing to see how it might "spoil some > vast eternal plan"[1] if a USB stick which Windows would show as > having no label, blkid/udev showed as having the label "LABEL42"? > > [1] to quote the song "If I were a rich man" > > I'm just trying to understand why this specific detail of bit-for-bit > compatibility is so important. Then you can ask question: Why we are serving LABEL information to users? Some users really use LABEL as identifier for themself, what is either stored on filesystem (in this case USB stick). And if somebody for some reason want to remove label (just because it does not contain data which label describe anymore), then user probably really want to use this information. LABEL is human readable identifier of particular filesystem on disk. If one system show one label and another system show another different label for same disk, then such information is useless. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers
On Thursday 09 November 2017 16:13:52 mario.limoncie...@dell.com wrote:
> > -Original Message-
> > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > Sent: Thursday, November 9, 2017 10:03 AM
> > To: Limonciello, Mario <mario_limoncie...@dell.com>
> > Cc: dvh...@infradead.org; Andy Shevchenko <andy.shevche...@gmail.com>;
> > LKML <linux-kernel@vger.kernel.org>; platform-driver-...@vger.kernel.org
> > Subject: Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial
> > probe to
> > dependent drivers
> >
> > On Friday 03 November 2017 11:27:22 Mario Limonciello wrote:
> > > dell-wmi and dell-smbios-wmi are dependent upon dell-wmi-descriptor
> > > finishing probe successfully to probe themselves.
> > >
> > > Currently if dell-wmi-descriptor fails probing in a non-recoverable way
> > > (such as invalid header) dell-wmi and dell-smbios-wmi will continue to
> > > try to redo probing due to deferred probing.
> > >
> > > To solve this have the dependent drivers query the dell-wmi-descriptor
> > > driver whether the descriptor has been determined valid. The possible
> > > results are:
> > > -EPROBE_DEFER: Descriptor not yet probed, dependent driver should wait
> > > and use deferred probing
> > > < 0: Descriptor probed, invalid. Dependent driver should return an
> > > error.
> > > 0: Successful descriptor probe, dependent driver can continue
> > >
> > > Successful descriptor probe still doesn't mean that the descriptor driver
> > > is necessarily bound at the time of initialization of dependent driver.
> > > Userspace can unbind the driver, so all methods used from driver
> > > should still be verified to return success values otherwise deferred
> > > probing be used.
> >
> > Darren, Andy, any comments on this patch?
> >
> > I think now it should work also those corner, but legit cases.
> >
> > > Signed-off-by: Mario Limonciello <mario.limoncie...@dell.com>
> > > ---
> > > drivers/platform/x86/dell-smbios-wmi.c | 4
> > > drivers/platform/x86/dell-wmi-descriptor.c | 11 +++
> > > drivers/platform/x86/dell-wmi-descriptor.h | 7 +++
> > > drivers/platform/x86/dell-wmi.c| 5 +
> > > 4 files changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/platform/x86/dell-smbios-wmi.c
> > > b/drivers/platform/x86/dell-
> > smbios-wmi.c
> > > index 35c13815b24c..3fa53fa174b2 100644
> > > --- a/drivers/platform/x86/dell-smbios-wmi.c
> > > +++ b/drivers/platform/x86/dell-smbios-wmi.c
> > > @@ -151,6 +151,10 @@ static int dell_smbios_wmi_probe(struct wmi_device
> > *wdev)
> > > if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
> > > return -ENODEV;
> > >
> > > + ret = dell_wmi_get_descriptor_valid();
> > > + if (ret)
> > > + return ret;
> > > +
> > > priv = devm_kzalloc(>dev, sizeof(struct wmi_smbios_priv),
> > > GFP_KERNEL);
> > > if (!priv)
> > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.c
> > b/drivers/platform/x86/dell-wmi-descriptor.c
> > > index 28ef5f37cfbf..e7f4c3a7bfc4 100644
> > > --- a/drivers/platform/x86/dell-wmi-descriptor.c
> > > +++ b/drivers/platform/x86/dell-wmi-descriptor.c
> > > @@ -26,9 +26,16 @@ struct descriptor_priv {
> > > u32 interface_version;
> > > u32 size;
> > > };
> > > +static int descriptor_valid = -EPROBE_DEFER;
> > > static LIST_HEAD(wmi_list);
> > > static DEFINE_MUTEX(list_mutex);
> > >
> > > +int dell_wmi_get_descriptor_valid(void)
> > > +{
> > > + return descriptor_valid;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dell_wmi_get_descriptor_valid);
> > > +
> > > bool dell_wmi_get_interface_version(u32 *version)
> > > {
> > > struct descriptor_priv *priv;
> > > @@ -91,6 +98,7 @@ static int dell_wmi_descriptor_probe(struct wmi_device
> > *wdev)
> > > if (obj->type != ACPI_TYPE_BUFFER) {
> > > dev_err(>dev, "Dell descriptor has wrong type\n");
> > > ret = -EINVAL;
> > > + descriptor_valid = ret;
> > > goto out;
> > > }
> > >
> > > @@ -102,6 +110,7 @@ static int dell_wmi_descriptor_probe(struct wmi_device
> > *wdev)
> > > "Dell descriptor buffer has unexpected length (%d)
Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers
On Thursday 09 November 2017 16:13:52 mario.limoncie...@dell.com wrote:
> > -Original Message-
> > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > Sent: Thursday, November 9, 2017 10:03 AM
> > To: Limonciello, Mario
> > Cc: dvh...@infradead.org; Andy Shevchenko ;
> > LKML ; platform-driver-...@vger.kernel.org
> > Subject: Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial
> > probe to
> > dependent drivers
> >
> > On Friday 03 November 2017 11:27:22 Mario Limonciello wrote:
> > > dell-wmi and dell-smbios-wmi are dependent upon dell-wmi-descriptor
> > > finishing probe successfully to probe themselves.
> > >
> > > Currently if dell-wmi-descriptor fails probing in a non-recoverable way
> > > (such as invalid header) dell-wmi and dell-smbios-wmi will continue to
> > > try to redo probing due to deferred probing.
> > >
> > > To solve this have the dependent drivers query the dell-wmi-descriptor
> > > driver whether the descriptor has been determined valid. The possible
> > > results are:
> > > -EPROBE_DEFER: Descriptor not yet probed, dependent driver should wait
> > > and use deferred probing
> > > < 0: Descriptor probed, invalid. Dependent driver should return an
> > > error.
> > > 0: Successful descriptor probe, dependent driver can continue
> > >
> > > Successful descriptor probe still doesn't mean that the descriptor driver
> > > is necessarily bound at the time of initialization of dependent driver.
> > > Userspace can unbind the driver, so all methods used from driver
> > > should still be verified to return success values otherwise deferred
> > > probing be used.
> >
> > Darren, Andy, any comments on this patch?
> >
> > I think now it should work also those corner, but legit cases.
> >
> > > Signed-off-by: Mario Limonciello
> > > ---
> > > drivers/platform/x86/dell-smbios-wmi.c | 4
> > > drivers/platform/x86/dell-wmi-descriptor.c | 11 +++
> > > drivers/platform/x86/dell-wmi-descriptor.h | 7 +++
> > > drivers/platform/x86/dell-wmi.c| 5 +
> > > 4 files changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/platform/x86/dell-smbios-wmi.c
> > > b/drivers/platform/x86/dell-
> > smbios-wmi.c
> > > index 35c13815b24c..3fa53fa174b2 100644
> > > --- a/drivers/platform/x86/dell-smbios-wmi.c
> > > +++ b/drivers/platform/x86/dell-smbios-wmi.c
> > > @@ -151,6 +151,10 @@ static int dell_smbios_wmi_probe(struct wmi_device
> > *wdev)
> > > if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
> > > return -ENODEV;
> > >
> > > + ret = dell_wmi_get_descriptor_valid();
> > > + if (ret)
> > > + return ret;
> > > +
> > > priv = devm_kzalloc(>dev, sizeof(struct wmi_smbios_priv),
> > > GFP_KERNEL);
> > > if (!priv)
> > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.c
> > b/drivers/platform/x86/dell-wmi-descriptor.c
> > > index 28ef5f37cfbf..e7f4c3a7bfc4 100644
> > > --- a/drivers/platform/x86/dell-wmi-descriptor.c
> > > +++ b/drivers/platform/x86/dell-wmi-descriptor.c
> > > @@ -26,9 +26,16 @@ struct descriptor_priv {
> > > u32 interface_version;
> > > u32 size;
> > > };
> > > +static int descriptor_valid = -EPROBE_DEFER;
> > > static LIST_HEAD(wmi_list);
> > > static DEFINE_MUTEX(list_mutex);
> > >
> > > +int dell_wmi_get_descriptor_valid(void)
> > > +{
> > > + return descriptor_valid;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dell_wmi_get_descriptor_valid);
> > > +
> > > bool dell_wmi_get_interface_version(u32 *version)
> > > {
> > > struct descriptor_priv *priv;
> > > @@ -91,6 +98,7 @@ static int dell_wmi_descriptor_probe(struct wmi_device
> > *wdev)
> > > if (obj->type != ACPI_TYPE_BUFFER) {
> > > dev_err(>dev, "Dell descriptor has wrong type\n");
> > > ret = -EINVAL;
> > > + descriptor_valid = ret;
> > > goto out;
> > > }
> > >
> > > @@ -102,6 +110,7 @@ static int dell_wmi_descriptor_probe(struct wmi_device
> > *wdev)
> > > "Dell descriptor buffer has unexpected length (%d)\n",
> > > obj->buffer.length);
> > > ret = -EINVAL;
> > > +
Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers
+ */
> +int dell_wmi_get_descriptor_valid(void);
> +
> bool dell_wmi_get_interface_version(u32 *version);
> bool dell_wmi_get_size(u32 *size);
>
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 54321080a30d..bb7c1e681792 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -655,10 +655,15 @@ static int dell_wmi_events_set_enabled(bool enable)
> static int dell_wmi_probe(struct wmi_device *wdev)
> {
> struct dell_wmi_priv *priv;
> + int ret;
>
> if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
> return -ENODEV;
Just one suggestion, is above check still needed in dell-wmi.c code?
I think that now it should be job of dell_wmi_get_descriptor_valid()
function to fully validate if dell wmi descriptor driver is able to
provide all needed information for dell-wmi driver.
> + ret = dell_wmi_get_descriptor_valid();
> + if (ret)
> + return ret;
> +
> priv = devm_kzalloc(
> >dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
> if (!priv)
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers
tor_valid(void);
> +
> bool dell_wmi_get_interface_version(u32 *version);
> bool dell_wmi_get_size(u32 *size);
>
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 54321080a30d..bb7c1e681792 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -655,10 +655,15 @@ static int dell_wmi_events_set_enabled(bool enable)
> static int dell_wmi_probe(struct wmi_device *wdev)
> {
> struct dell_wmi_priv *priv;
> + int ret;
>
> if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
> return -ENODEV;
Just one suggestion, is above check still needed in dell-wmi.c code?
I think that now it should be job of dell_wmi_get_descriptor_valid()
function to fully validate if dell wmi descriptor driver is able to
provide all needed information for dell-wmi driver.
> + ret = dell_wmi_get_descriptor_valid();
> + if (ret)
> + return ret;
> +
> priv = devm_kzalloc(
> >dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
> if (!priv)
--
Pali Rohár
pali.ro...@gmail.com
Re: bq2415x_charger: Use common error handling code in bq2415x_timer_work()
On Thursday 09 November 2017 14:04:19 SF Markus Elfring wrote: > > Better fix would be to display separate messages; user is probably > > interested in what failed... > > Which information (or wording) would you find more appropriate > at these places? Hi! Basically dropping your patch and instead of the "Unknown error" return to user reason why BQ2415X_BOOST_MODE_STATUS or BQ2415X_FAULT_STATUS commands failed. Or at least instead of the "Unknown error" write "Unknown error during BQ2415X_FAULT_STATUS". Basically I do not see any value in your patch. Current coding style pattern in that function is: do_something; if failed: print error; return; And your patch just changed some, but not *all* parts of code to: do_something; if failed: goto end_of_function If you are changing coding style, I would really suggest to change it on all places to let it consistent. Because your change introduces just inconsistency. -- Pali Rohár pali.ro...@gmail.com
Re: bq2415x_charger: Use common error handling code in bq2415x_timer_work()
On Thursday 09 November 2017 14:04:19 SF Markus Elfring wrote: > > Better fix would be to display separate messages; user is probably > > interested in what failed... > > Which information (or wording) would you find more appropriate > at these places? Hi! Basically dropping your patch and instead of the "Unknown error" return to user reason why BQ2415X_BOOST_MODE_STATUS or BQ2415X_FAULT_STATUS commands failed. Or at least instead of the "Unknown error" write "Unknown error during BQ2415X_FAULT_STATUS". Basically I do not see any value in your patch. Current coding style pattern in that function is: do_something; if failed: print error; return; And your patch just changed some, but not *all* parts of code to: do_something; if failed: goto end_of_function If you are changing coding style, I would really suggest to change it on all places to let it consistent. Because your change introduces just inconsistency. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH 1/2] platform/x86: dell-wmi-descriptor: check if memory was allocated
On Friday 03 November 2017 11:27:21 Mario Limonciello wrote:
> devm_kzalloc will return NULL pointer if no memory was allocated.
> This should be checked. This problem also existed when the driver
> was dell-wmi.c.
>
> Signed-off-by: Mario Limonciello <mario.limoncie...@dell.com>
Reviewed-by: Pali Rohár <pali.ro...@gmail.com>
> ---
> drivers/platform/x86/dell-wmi-descriptor.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/platform/x86/dell-wmi-descriptor.c
> b/drivers/platform/x86/dell-wmi-descriptor.c
> index 3204c408e261..28ef5f37cfbf 100644
> --- a/drivers/platform/x86/dell-wmi-descriptor.c
> +++ b/drivers/platform/x86/dell-wmi-descriptor.c
> @@ -121,6 +121,11 @@ static int dell_wmi_descriptor_probe(struct wmi_device
> *wdev)
> priv = devm_kzalloc(>dev, sizeof(struct descriptor_priv),
> GFP_KERNEL);
>
> + if (!priv) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> priv->interface_version = buffer[2];
> priv->size = buffer[3];
> ret = 0;
--
Pali Rohár
pali.ro...@gmail.com
Re: [PATCH 1/2] platform/x86: dell-wmi-descriptor: check if memory was allocated
On Friday 03 November 2017 11:27:21 Mario Limonciello wrote:
> devm_kzalloc will return NULL pointer if no memory was allocated.
> This should be checked. This problem also existed when the driver
> was dell-wmi.c.
>
> Signed-off-by: Mario Limonciello
Reviewed-by: Pali Rohár
> ---
> drivers/platform/x86/dell-wmi-descriptor.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/platform/x86/dell-wmi-descriptor.c
> b/drivers/platform/x86/dell-wmi-descriptor.c
> index 3204c408e261..28ef5f37cfbf 100644
> --- a/drivers/platform/x86/dell-wmi-descriptor.c
> +++ b/drivers/platform/x86/dell-wmi-descriptor.c
> @@ -121,6 +121,11 @@ static int dell_wmi_descriptor_probe(struct wmi_device
> *wdev)
> priv = devm_kzalloc(>dev, sizeof(struct descriptor_priv),
> GFP_KERNEL);
>
> + if (!priv) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> priv->interface_version = buffer[2];
> priv->size = buffer[3];
> ret = 0;
--
Pali Rohár
pali.ro...@gmail.com
Re: Linux & FAT32 label
On Tuesday 07 November 2017 12:28:41 Theodore Ts'o wrote: > On Sun, Nov 05, 2017 at 10:12:49PM +0100, Pali Rohár wrote: > > Easy way how to achieve this situation: > > > > 1. use mkdosfs to format hard disk to FAT32 with label LABEL42 > > > > 2. boot Windows 10 (or XP) and set label of that FAT32 partition to > > empty (via Explorer GUI) > > > > 3. profit > > > > You would have stored LABEL42 in boot sector and no label in root > > directory. Windows handle this situation as there is no label. > > But why should we *care*? FAT is Microsoft's filesystem and the only usage of it on Linux is due to interoperability with different non-Linux systems. So here we should implement FAT in the similar/same way as other systems. It does not make sense to implement it differently and specially in non-compatible way. Because it lost reason what is primary usage of the FAT on Linux. -- Pali Rohár pali.ro...@gmail.com
Re: Linux & FAT32 label
On Tuesday 07 November 2017 12:28:41 Theodore Ts'o wrote: > On Sun, Nov 05, 2017 at 10:12:49PM +0100, Pali Rohár wrote: > > Easy way how to achieve this situation: > > > > 1. use mkdosfs to format hard disk to FAT32 with label LABEL42 > > > > 2. boot Windows 10 (or XP) and set label of that FAT32 partition to > > empty (via Explorer GUI) > > > > 3. profit > > > > You would have stored LABEL42 in boot sector and no label in root > > directory. Windows handle this situation as there is no label. > > But why should we *care*? FAT is Microsoft's filesystem and the only usage of it on Linux is due to interoperability with different non-Linux systems. So here we should implement FAT in the similar/same way as other systems. It does not make sense to implement it differently and specially in non-compatible way. Because it lost reason what is primary usage of the FAT on Linux. -- Pali Rohár pali.ro...@gmail.com
