Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Marius Zachmann <mail@mariuszachmann.de>
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Jean Delvare <jdelvare@suse.com>, Jonathan Corbet <corbet@lwn.net>,
        linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hwmon: corsair-cpro: Change to hid driver
Date:   Wed, 15 Jul 2020 08:59:52 +0200
Message-ID: <1747396.iuIVfOqGsT@marius>
In-Reply-To: <20200715023622.GA164279@roeck-us.net>
References: <20200714105230.3126-1-mail@mariuszachmann.de> <20200715023622.GA164279@roeck-us.net>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 15.07.20 at 04:36:22 CEST, Guenter Roeck wrote
> On Tue, Jul 14, 2020 at 12:52:30PM +0200, Marius Zachmann wrote:
> > I found a project which uses hidraw to communicate with the device.
> > Because I do not want to break any existing userspace code, I
> > changed this to a hid driver, so hidraw can still be used.
> > Do I need to include the hid maintainers for the undo in hid-quirks?
> > 
> That changelog needs some improvements. It should state what change
> was made, and why, but not include any questions or personal statements
> such as "I found ...".
> 
> You never really explained why you had changed the driver from hid to
> usb. Maybe you can explain that decision now ?
> 
> Thanks,
> Guenter
> 

This device does not use numbered hid input reports. As far as I know
it is not possible to determine from which command a response was
requested. This being a usb driver would not allow any other driver
to send data to the device.
In the first (hid) version I did not use hid reports and got
the usb_device directly from the hid_device which is not intended.
In this version, requests are made via hid_hw_output_report, waited
for and received via raw_event.
If one uses hidraw and this driver at the same time, it still could
be possible to get switched responses but there is nothing I can
do about it, as far as I know. This is only about giving the
possibility to unload the driver and using a userspace driver
if one chooses.

Greetings
Marius

> > Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
> > ---
> >  drivers/hid/hid-quirks.c     |   2 -
> >  drivers/hwmon/Kconfig        |   4 +-
> >  drivers/hwmon/corsair-cpro.c | 114 ++++++++++++++++++++++++++---------
> >  3 files changed, 86 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> > index 7b7bc7737c53..ca8b5c261c7c 100644
> > --- a/drivers/hid/hid-quirks.c
> > +++ b/drivers/hid/hid-quirks.c
> > @@ -699,8 +699,6 @@ static const struct hid_device_id hid_ignore_list[] = {
> >  	{ HID_USB_DEVICE(USB_VENDOR_ID_AXENTIA, USB_DEVICE_ID_AXENTIA_FM_RADIO) },
> >  	{ HID_USB_DEVICE(USB_VENDOR_ID_BERKSHIRE, USB_DEVICE_ID_BERKSHIRE_PCWD) },
> >  	{ HID_USB_DEVICE(USB_VENDOR_ID_CIDC, 0x0103) },
> > -	{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, 0x0c10) },
> > -	{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, 0x1d00) },
> >  	{ HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_RADIO_SI470X) },
> >  	{ HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_RADIO_SI4713) },
> >  	{ HID_USB_DEVICE(USB_VENDOR_ID_CMEDIA, USB_DEVICE_ID_CM109) },
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 8b046a5dfa40..c603d8c8e3d2 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -441,7 +441,7 @@ config SENSORS_BT1_PVT_ALARMS
> > 
> >  config SENSORS_CORSAIR_CPRO
> >  	tristate "Corsair Commander Pro controller"
> > -	depends on USB
> > +	depends on HID
> >  	help
> >  	  If you say yes here you get support for the Corsair Commander Pro
> >  	  controller.
> > @@ -1716,7 +1716,7 @@ config SENSORS_ADS7871
> > 
> >  config SENSORS_AMC6821
> >  	tristate "Texas Instruments AMC6821"
> > -	depends on I2C
> > +	depends on I2C
> >  	help
> >  	  If you say yes here you get support for the Texas Instruments
> >  	  AMC6821 hardware monitoring chips.
> > diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> > index fe625190e3a1..4310ee5aca24 100644
> > --- a/drivers/hwmon/corsair-cpro.c
> > +++ b/drivers/hwmon/corsair-cpro.c
> > @@ -5,13 +5,14 @@
> >   */
> > 
> >  #include <linux/bitops.h>
> > +#include <linux/completion.h>
> > +#include <linux/hid.h>
> >  #include <linux/hwmon.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > -#include <linux/usb.h>
> > 
> >  #define USB_VENDOR_ID_CORSAIR			0x1b1c
> >  #define USB_PRODUCT_ID_CORSAIR_COMMANDERPRO	0x0c10
> > @@ -62,7 +63,8 @@
> >  #define NUM_TEMP_SENSORS	4
> > 
> >  struct ccp_device {
> > -	struct usb_device *udev;
> > +	struct hid_device *hdev;
> > +	struct completion wait_input_report;
> >  	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
> >  	u8 *buffer;
> >  	int pwm[6];
> > @@ -75,7 +77,7 @@ struct ccp_device {
> >  /* send command, check for error in response, response in ccp->buffer */
> >  static int send_usb_cmd(struct ccp_device *ccp, u8 command, u8 byte1, u8 byte2, u8 byte3)
> >  {
> > -	int actual_length;
> > +	unsigned long t;
> >  	int ret;
> > 
> >  	memset(ccp->buffer, 0x00, OUT_BUFFER_SIZE);
> > @@ -84,26 +86,39 @@ static int send_usb_cmd(struct ccp_device *ccp, u8 command, u8 byte1, u8 byte2,
> >  	ccp->buffer[2] = byte2;
> >  	ccp->buffer[3] = byte3;
> > 
> > -	ret = usb_bulk_msg(ccp->udev, usb_sndintpipe(ccp->udev, 2), ccp->buffer, OUT_BUFFER_SIZE,
> > -			   &actual_length, 1000);
> > -	if (ret)
> > -		return ret;
> > +	reinit_completion(&ccp->wait_input_report);
> > 
> > -	/* response needs to be received every time */
> > -	ret = usb_bulk_msg(ccp->udev, usb_rcvintpipe(ccp->udev, 1), ccp->buffer, IN_BUFFER_SIZE,
> > -			   &actual_length, 1000);
> > -	if (ret)
> > +	ret = hid_hw_output_report(ccp->hdev, ccp->buffer, OUT_BUFFER_SIZE);
> > +	if (ret < 0)
> >  		return ret;
> > 
> > +	t = wait_for_completion_timeout(&ccp->wait_input_report, msecs_to_jiffies(300));
> > +	if (!t)
> > +		return -ETIMEDOUT;
> > +
> >  	/* first byte of response is error code */
> >  	if (ccp->buffer[0] != 0x00) {
> > -		dev_dbg(&ccp->udev->dev, "device response error: %d", ccp->buffer[0]);
> > +		hid_dbg(ccp->hdev, "device response error: %d", ccp->buffer[0]);
> >  		return -EIO;
> >  	}
> > 
> >  	return 0;
> >  }
> > 
> > +static int ccp_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *data, int size)
> > +{
> > +	struct ccp_device *ccp = hid_get_drvdata(hdev);
> > +
> > +	/* only copy buffer when requested */
> > +	if (completion_done(&ccp->wait_input_report))
> > +		return 0;
> > +
> > +	memcpy(ccp->buffer, data, min(IN_BUFFER_SIZE, size));
> > +	complete(&ccp->wait_input_report);
> > +
> > +	return 0;
> > +}
> > +
> >  /* requests and returns single data values depending on channel */
> >  static int get_data(struct ccp_device *ccp, int command, int channel)
> >  {
> > @@ -437,57 +452,96 @@ static int get_temp_cnct(struct ccp_device *ccp)
> >  	return 0;
> >  }
> > 
> > -static int ccp_probe(struct usb_interface *intf, const struct usb_device_id *id)
> > +static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >  {
> >  	struct device *hwmon_dev;
> >  	struct ccp_device *ccp;
> >  	int ret;
> > 
> > -	ccp = devm_kzalloc(&intf->dev, sizeof(*ccp), GFP_KERNEL);
> > +	ccp = devm_kzalloc(&hdev->dev, sizeof(*ccp), GFP_KERNEL);
> >  	if (!ccp)
> >  		return -ENOMEM;
> > 
> > -	ccp->buffer = devm_kmalloc(&intf->dev, OUT_BUFFER_SIZE, GFP_KERNEL);
> > +	ccp->buffer = devm_kmalloc(&hdev->dev, OUT_BUFFER_SIZE, GFP_KERNEL);
> >  	if (!ccp->buffer)
> >  		return -ENOMEM;
> > 
> > +	ret = hid_parse(hdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = hid_hw_open(hdev);
> > +	if (ret)
> > +		goto out_hw_stop;
> > +
> > +	ccp->hdev = hdev;
> > +	hid_set_drvdata(hdev, ccp);
> >  	mutex_init(&ccp->mutex);
> > +	init_completion(&ccp->wait_input_report);
> > 
> > -	ccp->udev = interface_to_usbdev(intf);
> > +	hid_device_io_start(hdev);
> > 
> >  	/* temp and fan connection status only updates when device is powered on */
> >  	ret = get_temp_cnct(ccp);
> >  	if (ret)
> > -		return ret;
> > +		goto out_hw_close;
> > 
> >  	ret = get_fan_cnct(ccp);
> >  	if (ret)
> > -		return ret;
> > -
> > -	hwmon_dev = devm_hwmon_device_register_with_info(&intf->dev, "corsaircpro", ccp,
> > +		goto out_hw_close;
> > +	hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "corsaircpro", ccp,
> >  							 &ccp_chip_info, 0);
> > +	if (IS_ERR(hwmon_dev)) {
> > +		ret = PTR_ERR(hwmon_dev);
> > +		goto out_hw_close;
> > +	}
> > +
> > +	return 0;
> > 
> > -	return PTR_ERR_OR_ZERO(hwmon_dev);
> > +out_hw_close:
> > +	hid_hw_close(hdev);
> > +out_hw_stop:
> > +	hid_hw_stop(hdev);
> > +	return ret;
> >  }
> > 
> > -static void ccp_disconnect(struct usb_interface *intf)
> > +static void ccp_remove(struct hid_device *hdev)
> >  {
> > +	hid_hw_close(hdev);
> > +	hid_hw_stop(hdev);
> >  }
> > 
> > -static const struct usb_device_id ccp_devices[] = {
> > -	{ USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_COMMANDERPRO) },
> > -	{ USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_1000D) },
> > +static const struct hid_device_id ccp_devices[] = {
> > +	{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_COMMANDERPRO) },
> > +	{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_1000D) },
> >  	{ }
> >  };
> > 
> > -static struct usb_driver ccp_driver = {
> > +static struct hid_driver ccp_driver = {
> >  	.name = "corsair-cpro",
> > +	.id_table = ccp_devices,
> >  	.probe = ccp_probe,
> > -	.disconnect = ccp_disconnect,
> > -	.id_table = ccp_devices
> > +	.remove = ccp_remove,
> > +	.raw_event = ccp_raw_event,
> >  };
> > 
> > -MODULE_DEVICE_TABLE(usb, ccp_devices);
> > +MODULE_DEVICE_TABLE(hid, ccp_devices);
> >  MODULE_LICENSE("GPL");
> > 
> > -module_usb_driver(ccp_driver);
> > +static int __init ccp_init(void)
> > +{
> > +	return hid_register_driver(&ccp_driver);
> > +}
> > +
> > +static void __exit ccp_exit(void)
> > +{
> > +	hid_unregister_driver(&ccp_driver);
> > +}
> > +
> > +/* make sure, it is loaded after hid */
> > +late_initcall(ccp_init);
> > +module_exit(ccp_exit);
> > --
> > 2.27.0
>