[PATCH] HID: steam: use hid_device.driver_data instead of hid_set_drvdata()

[PATCH] HID: steam: use hid_device.driver_data instead of hid_set_drvdata()

[Rodrigo Rivas Costa]

When creating the low-level hidraw device, the reference to steam_device
was stored using hid_set_drvdata(). But this value is not guaranteed to
be kept when set before calling probe. If this pointer is reset, it
crashes when opening the emulated hidraw device.

It looks like hid_set_drvdata() is for users "avobe" this hid_device,
while hid_device.driver_data it for users "below" this one.

In this case, we are creating a virtual hidraw device, so we must use
hid_device.driver_data.

Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
---

This patch is to be applied over hid/for-4.18/hid-steam. Is this the
proper way to signal it?

I don't know exactly when the problem started. I am pretty sure that it
worked with 4.16.2, but it failed with 4.16.9. Or maybe it is caused by
upgrading the firmware of the controller, although the protocol seems
identical.

 drivers/hid/hid-steam.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index cb86cc834201..0422ec2b13d2 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -573,7 +573,7 @@ static bool steam_is_valve_interface(struct hid_device *hdev)
 
 static int steam_client_ll_parse(struct hid_device *hdev)
 {
-	struct steam_device *steam = hid_get_drvdata(hdev);
+	struct steam_device *steam = hdev->driver_data;
 
 	return hid_parse_report(hdev, steam->hdev->dev_rdesc,
 			steam->hdev->dev_rsize);
@@ -590,7 +590,7 @@ static void steam_client_ll_stop(struct hid_device *hdev)
 
 static int steam_client_ll_open(struct hid_device *hdev)
 {
-	struct steam_device *steam = hid_get_drvdata(hdev);
+	struct steam_device *steam = hdev->driver_data;
 	int ret;
 
 	ret = hid_hw_open(steam->hdev);
@@ -605,7 +605,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
 
 static void steam_client_ll_close(struct hid_device *hdev)
 {
-	struct steam_device *steam = hid_get_drvdata(hdev);
+	struct steam_device *steam = hdev->driver_data;
 
 	mutex_lock(&steam->mutex);
 	steam->client_opened = false;
@@ -623,7 +623,7 @@ static int steam_client_ll_raw_request(struct hid_device *hdev,
 				size_t count, unsigned char report_type,
 				int reqtype)
 {
-	struct steam_device *steam = hid_get_drvdata(hdev);
+	struct steam_device *steam = hdev->driver_data;
 
 	return hid_hw_raw_request(steam->hdev, reportnum, buf, count,
 			report_type, reqtype);
@@ -710,7 +710,7 @@ static int steam_probe(struct hid_device *hdev,
 		ret = PTR_ERR(steam->client_hdev);
 		goto client_hdev_fail;
 	}
-	hid_set_drvdata(steam->client_hdev, steam);
+	steam->client_hdev->driver_data = steam;
 
 	/*
 	 * With the real steam controller interface, do not connect hidraw.
-- 
2.17.0

Re: [PATCH] HID: steam: use hid_device.driver_data instead of hid_set_drvdata()

[Rodrigo Rivas Costa]

Hi, all!

Could you check my previous patch to see if it makes any sense?
My machine currently crashes without it, but I'm not sure why. If you
think it is worth it I can try and bisect it.

Regards
--
Rodrigo Rivas Costa.


On Tue, May 22, 2018 at 10:10:06PM +0200, Rodrigo Rivas Costa wrote:
> When creating the low-level hidraw device, the reference to steam_device
> was stored using hid_set_drvdata(). But this value is not guaranteed to
> be kept when set before calling probe. If this pointer is reset, it
> crashes when opening the emulated hidraw device.
> 
> It looks like hid_set_drvdata() is for users "avobe" this hid_device,
> while hid_device.driver_data it for users "below" this one.
> 
> In this case, we are creating a virtual hidraw device, so we must use
> hid_device.driver_data.
> 
> Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
> ---
> 
> This patch is to be applied over hid/for-4.18/hid-steam. Is this the
> proper way to signal it?
> 
> I don't know exactly when the problem started. I am pretty sure that it
> worked with 4.16.2, but it failed with 4.16.9. Or maybe it is caused by
> upgrading the firmware of the controller, although the protocol seems
> identical.
> 
>  drivers/hid/hid-steam.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index cb86cc834201..0422ec2b13d2 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -573,7 +573,7 @@ static bool steam_is_valve_interface(struct hid_device *hdev)
>  
>  static int steam_client_ll_parse(struct hid_device *hdev)
>  {
> -	struct steam_device *steam = hid_get_drvdata(hdev);
> +	struct steam_device *steam = hdev->driver_data;
>  
>  	return hid_parse_report(hdev, steam->hdev->dev_rdesc,
>  			steam->hdev->dev_rsize);
> @@ -590,7 +590,7 @@ static void steam_client_ll_stop(struct hid_device *hdev)
>  
>  static int steam_client_ll_open(struct hid_device *hdev)
>  {
> -	struct steam_device *steam = hid_get_drvdata(hdev);
> +	struct steam_device *steam = hdev->driver_data;
>  	int ret;
>  
>  	ret = hid_hw_open(steam->hdev);
> @@ -605,7 +605,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
>  
>  static void steam_client_ll_close(struct hid_device *hdev)
>  {
> -	struct steam_device *steam = hid_get_drvdata(hdev);
> +	struct steam_device *steam = hdev->driver_data;
>  
>  	mutex_lock(&steam->mutex);
>  	steam->client_opened = false;
> @@ -623,7 +623,7 @@ static int steam_client_ll_raw_request(struct hid_device *hdev,
>  				size_t count, unsigned char report_type,
>  				int reqtype)
>  {
> -	struct steam_device *steam = hid_get_drvdata(hdev);
> +	struct steam_device *steam = hdev->driver_data;
>  
>  	return hid_hw_raw_request(steam->hdev, reportnum, buf, count,
>  			report_type, reqtype);
> @@ -710,7 +710,7 @@ static int steam_probe(struct hid_device *hdev,
>  		ret = PTR_ERR(steam->client_hdev);
>  		goto client_hdev_fail;
>  	}
> -	hid_set_drvdata(steam->client_hdev, steam);
> +	steam->client_hdev->driver_data = steam;
>  
>  	/*
>  	 * With the real steam controller interface, do not connect hidraw.
> -- 
> 2.17.0
> 

Re: [PATCH] HID: steam: use hid_device.driver_data instead of hid_set_drvdata()

[Jiri Kosina]

On Fri, 1 Jun 2018, Rodrigo Rivas Costa wrote:

> Could you check my previous patch to see if it makes any sense?
> My machine currently crashes without it, but I'm not sure why. If you
> think it is worth it I can try and bisect it.

I sent a pull request to Linus earlier today, I'll stage this for 2nd wave 
so that it still goes in for 4.18.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: steam: use hid_device.driver_data instead of hid_set_drvdata()

[Mariusz Ceier]

[-- Attachment #1: Type: text/plain, Size: 3483 bytes --]

Hello Rodrigo,
   I confirm that this patch fixes the steam controller related crash
for me. I don't know if it's correct, but at least it's:

Tested-by: Mariusz Ceier <mceier+kernel@gmail.com>

I'm attaching the backtrace of crash I was getting before applying the patch.

Best regards,
Mariusz Ceier


On 22 May 2018 at 22:10, Rodrigo Rivas Costa
<rodrigorivascosta@gmail.com> wrote:
> When creating the low-level hidraw device, the reference to steam_device
> was stored using hid_set_drvdata(). But this value is not guaranteed to
> be kept when set before calling probe. If this pointer is reset, it
> crashes when opening the emulated hidraw device.
>
> It looks like hid_set_drvdata() is for users "avobe" this hid_device,
> while hid_device.driver_data it for users "below" this one.
>
> In this case, we are creating a virtual hidraw device, so we must use
> hid_device.driver_data.
>
> Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
> ---
>
> This patch is to be applied over hid/for-4.18/hid-steam. Is this the
> proper way to signal it?
>
> I don't know exactly when the problem started. I am pretty sure that it
> worked with 4.16.2, but it failed with 4.16.9. Or maybe it is caused by
> upgrading the firmware of the controller, although the protocol seems
> identical.
>
>  drivers/hid/hid-steam.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index cb86cc834201..0422ec2b13d2 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -573,7 +573,7 @@ static bool steam_is_valve_interface(struct hid_device *hdev)
>
>  static int steam_client_ll_parse(struct hid_device *hdev)
>  {
> -       struct steam_device *steam = hid_get_drvdata(hdev);
> +       struct steam_device *steam = hdev->driver_data;
>
>         return hid_parse_report(hdev, steam->hdev->dev_rdesc,
>                         steam->hdev->dev_rsize);
> @@ -590,7 +590,7 @@ static void steam_client_ll_stop(struct hid_device *hdev)
>
>  static int steam_client_ll_open(struct hid_device *hdev)
>  {
> -       struct steam_device *steam = hid_get_drvdata(hdev);
> +       struct steam_device *steam = hdev->driver_data;
>         int ret;
>
>         ret = hid_hw_open(steam->hdev);
> @@ -605,7 +605,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
>
>  static void steam_client_ll_close(struct hid_device *hdev)
>  {
> -       struct steam_device *steam = hid_get_drvdata(hdev);
> +       struct steam_device *steam = hdev->driver_data;
>
>         mutex_lock(&steam->mutex);
>         steam->client_opened = false;
> @@ -623,7 +623,7 @@ static int steam_client_ll_raw_request(struct hid_device *hdev,
>                                 size_t count, unsigned char report_type,
>                                 int reqtype)
>  {
> -       struct steam_device *steam = hid_get_drvdata(hdev);
> +       struct steam_device *steam = hdev->driver_data;
>
>         return hid_hw_raw_request(steam->hdev, reportnum, buf, count,
>                         report_type, reqtype);
> @@ -710,7 +710,7 @@ static int steam_probe(struct hid_device *hdev,
>                 ret = PTR_ERR(steam->client_hdev);
>                 goto client_hdev_fail;
>         }
> -       hid_set_drvdata(steam->client_hdev, steam);
> +       steam->client_hdev->driver_data = steam;
>
>         /*
>          * With the real steam controller interface, do not connect hidraw.
> --
> 2.17.0
>

[-- Attachment #2: steam_backtrace.txt --]
[-- Type: text/plain, Size: 6158 bytes --]

[ 4004.866471] BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
[ 4004.866475] PGD 800000086ade5067 P4D 800000086ade5067 PUD 0 
[ 4004.866479] Oops: 0000 [#1] PREEMPT SMP PTI
[ 4004.866481] CPU: 2 PID: 4884 Comm: CSteamControlle Tainted: G        W         4.17.0-amdgpu-dev+ #1
[ 4004.866482] Hardware name: Gigabyte Technology Co., Ltd. Z170X-Gaming 7/Z170X-Gaming 7, BIOS F22j 01/11/2018
[ 4004.866486] RIP: 0010:steam_client_ll_open+0x17/0x50
[ 4004.866487] Code: e8 9e cb e0 ff 4c 89 e7 e8 d6 6a ff ff eb c0 0f 1f 40 00 e8 1b 09 26 00 55 48 89 e5 41 54 53 48 83 ec 08 48 8b 9f 68 19 00 00 <48> 8b 7b 18 e8 40 6a ff ff 85 c0 75 1e 4c 8d 63 28 89 45 ec 4c 89 
[ 4004.866510] RSP: 0018:ffffc0a841a5bb18 EFLAGS: 00010296
[ 4004.866511] RAX: ffffffffbdba0d50 RBX: 0000000000000000 RCX: 0000000000000001
[ 4004.866512] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff9c77752ee000
[ 4004.866513] RBP: ffffc0a841a5bb30 R08: ffff9c777a002d80 R09: ffff9c776b169800
[ 4004.866514] R10: ffff9c7779a1d025 R11: 0034776172646968 R12: ffff9c77752efba8
[ 4004.866515] R13: ffff9c776d256e00 R14: ffff9c7774c2b0c0 R15: ffffffffbd414750
[ 4004.866517] FS:  0000000000000000(0000) GS:ffff9c779dd00000(0063) knlGS:00000000e65cab40
[ 4004.866518] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
[ 4004.866519] CR2: 0000000000000018 CR3: 0000000864614004 CR4: 00000000003606e0
[ 4004.866521] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 4004.866522] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 4004.866522] Call Trace:
[ 4004.866526]  ? kmem_cache_alloc_trace+0x169/0x1c0
[ 4004.866528]  hid_hw_open+0x4c/0x70
[ 4004.866530]  hidraw_open+0xae/0x1d0
[ 4004.866532]  ? cdev_put.part.1+0x20/0x20
[ 4004.866534]  chrdev_open+0xa6/0x1c0
[ 4004.866536]  do_dentry_open+0x1bd/0x2f0
[ 4004.866538]  vfs_open+0x4f/0x80
[ 4004.866540]  do_last+0x4da/0x1240
[ 4004.866542]  ? inode_permission+0x54/0x190
[ 4004.866544]  ? path_init+0x19c/0x330
[ 4004.866546]  path_openat+0xa0/0x300
[ 4004.866548]  ? putname+0x4c/0x60
[ 4004.866550]  do_filp_open+0x9b/0x110
[ 4004.866552]  ? __check_object_size+0xa4/0x1b0
[ 4004.866554]  do_sys_open+0x1ba/0x250
[ 4004.866556]  ? do_filp_open+0x5/0x110
[ 4004.866558]  ? do_sys_open+0x1ba/0x250
[ 4004.866560]  __ia32_compat_sys_openat+0x1d/0x20
[ 4004.866562]  do_fast_syscall_32+0xb1/0x2c0
[ 4004.866565]  entry_SYSENTER_compat+0x84/0x96
[ 4004.866566] RIP: 0023:0xf7f79c59
[ 4004.866567] Code: 85 d2 74 02 89 0a 5b 5d c3 8b 04 24 c3 8b 0c 24 c3 8b 1c 24 c3 8b 3c 24 c3 90 90 90 90 90 90 90 90 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 eb 0d 90 90 90 90 90 90 90 90 90 90 90 90 
[ 4004.866589] RSP: 002b:00000000e65c88a0 EFLAGS: 00000282 ORIG_RAX: 0000000000000127
[ 4004.866591] RAX: ffffffffffffffda RBX: 00000000ffffff9c RCX: 00000000ee8650fc
[ 4004.866592] RDX: 0000000000000002 RSI: 0000000000000000 RDI: 00000000f7c11000
[ 4004.866593] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[ 4004.866594] R10: 0000000000000000 R11: 0000000000000282 R12: 0000000000000000
[ 4004.866595] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 4004.866596] Modules linked in: rpcsec_gss_krb5 x86_pkg_temp_thermal kvm_intel kvm snd_hda_codec_hdmi snd_hda_codec_ca0132 snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep irqbypass snd_pcm crc32c_intel ghash_clmulni_intel mxm_wmi snd_timer cryptd alx wmi snd video soundcore coretemp efivarfs dm_zero dm_thin_pool dm_persistent_data dm_bio_prison dm_log_userspace dm_flakey dm_delay virtio_pci virtio_scsi virtio_blk virtio_console virtio_balloon sha512_generic libiscsi scsi_transport_iscsi tulip cxgb3 cxgb cxgb4 vxge bonding vxlan ip6_udp_tunnel udp_tunnel macvlan vmxnet3 virtio_net net_failover failover virtio_ring virtio tg3 libphy sky2 r8169 pcnet32 mii e1000 bnx2 atl1c fuse xfs jfs reiserfs btrfs zstd_decompress zstd_compress xxhash linear raid10 raid1 raid0 dm_raid raid456 async_raid6_recov async_memcpy
[ 4004.866634]  async_pq async_xor async_tx xor raid6_pq dm_mirror dm_region_hash dm_log firewire_core crc_itu_t sl811_hcd usb_storage aic94xx libsas lpfc qla2xxx megaraid_sas megaraid_mbox megaraid_mm aacraid sx8 hpsa 3w_9xxx 3w_xxxx mptsas scsi_transport_sas mptfc scsi_transport_fc mptspi mptscsih mptbase sym53c8xx initio arcmsr aic7xxx aic79xx scsi_transport_spi sr_mod cdrom sg pdc_adma sata_inic162x sata_mv ata_piix sata_qstor sata_vsc sata_uli sata_sis sata_sx4 sata_nv sata_via sata_svw sata_sil24 sata_sil sata_promise pata_via pata_jmicron pata_marvell pata_sis pata_netcell pata_pdc202xx_old pata_atiixp pata_amd pata_ali pata_it8213 pata_pcmcia pata_serverworks pata_oldpiix pata_artop pata_it821x pata_hpt3x2n pata_hpt3x3 pata_hpt37x pata_hpt366 pata_cmd64x pata_sil680 pata_pdc2027x sd_mod ahci
[ 4004.866669]  libahci
[ 4004.866671] CR2: 0000000000000018
[ 4004.866673] ---[ end trace 9c22eadca9234307 ]---
[ 4004.907301] snd_hda_intel 0000:00:1f.3: Unstable LPIB (351688 >= 176400); disabling LPIB delay counting
[ 4004.963375] RIP: 0010:steam_client_ll_open+0x17/0x50
[ 4004.963378] Code: e8 9e cb e0 ff 4c 89 e7 e8 d6 6a ff ff eb c0 0f 1f 40 00 e8 1b 09 26 00 55 48 89 e5 41 54 53 48 83 ec 08 48 8b 9f 68 19 00 00 <48> 8b 7b 18 e8 40 6a ff ff 85 c0 75 1e 4c 8d 63 28 89 45 ec 4c 89 
[ 4004.963404] RSP: 0018:ffffc0a841a5bb18 EFLAGS: 00010296
[ 4004.963405] RAX: ffffffffbdba0d50 RBX: 0000000000000000 RCX: 0000000000000001
[ 4004.963407] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff9c77752ee000
[ 4004.963408] RBP: ffffc0a841a5bb30 R08: ffff9c777a002d80 R09: ffff9c776b169800
[ 4004.963409] R10: ffff9c7779a1d025 R11: 0034776172646968 R12: ffff9c77752efba8
[ 4004.963410] R13: ffff9c776d256e00 R14: ffff9c7774c2b0c0 R15: ffffffffbd414750
[ 4004.963412] FS:  0000000000000000(0000) GS:ffff9c779dd00000(0063) knlGS:00000000e65cab40
[ 4004.963413] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
[ 4004.963414] CR2: 0000000000000018 CR3: 0000000864614005 CR4: 00000000003606e0
[ 4004.963415] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 4004.963416] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400