Re: [PATCH v5 3/4] SFH: Transport Driver to add support of AMD Sensor Fusion Hub (SFH
flat view
From: Andy Shevchenko <hidden>
Date: 2020-05-29 14:20:49
Also in:
linux-iio, lkml
On Fri, May 29, 2020 at 4:43 PM Sandeep Singh [off-list ref] wrote:
From: Sandeep Singh <redacted> This part of module will provide the interaction between HID framework and client driver.This modules will registered client driver with HID framework.
...
Signed-off-by: Sandeep Singh <redacted> Signed-off-by: Nehal Shah <redacted>
Use proper sequence and Co-developed-by if applicable.
Reported-by: kbuild test robot <redacted> Reported-by: Dan Carpenter <redacted>
What has been reported? Absence of the drivers? ...
quoted hunk ↗ jump to hunk
+#include <linux/init.h> +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/workqueue.h> +#include <linux/hid.h> +#include <linux/list.h> +#include <linux/platform_device.h> +#include <linux/acpi.h> +#include <linux/dma-mapping.h>
Keep ordered?
quoted hunk ↗ jump to hunk
+#include "amdsfh-hid.h" +#include "amd_mp2_pcie.h" +#include "hid_descriptor/amd_sfh_hid_descriptor.h"
...
quoted hunk ↗ jump to hunk
+void amd_sfh_get_report(struct hid_device *hid, int report_id, + int report_type) +{ + struct amdtp_hid_data *hid_data = hid->driver_data; + struct amdtp_cl_data *cli_data = hid_data->cli_data; + int i; + + for (i = 0; i < cli_data->num_hid_devices; i++) { + if (cli_data->hid_sensor_hubs[i] == hid) {
+ struct request_list *new_request = kzalloc + (sizeof(*new_request), GFP_KERNEL);
Bad indentation.
+ new_request->current_index = i;
Huh? NULL pointer dereference!
quoted hunk ↗ jump to hunk
+ new_request->sensor_idx = cli_data->sensor_idx[i]; + new_request->hid = hid; + new_request->report_type = report_type; + new_request->report_id = report_id; + cli_data->report_id[i] = report_id; + cli_data->request_done[i] = false; + list_add(&new_request->list, &req_list.list); + break; + } + } + schedule_delayed_work(&cli_data->work, 0); +} +EXPORT_SYMBOL(amd_sfh_get_report);
...
quoted hunk ↗ jump to hunk
+static void amd_sfh_work_buffer(struct work_struct *work) +{ + struct amdtp_cl_data *cli_data = cl_data_context; + unsigned int i;
+ u8 report_size = 0;
Redundant assignment.
quoted hunk ↗ jump to hunk
+ for (i = 0; i < cli_data->num_hid_devices; i++) { + report_size = get_input_report(cli_data->sensor_idx[i], + cli_data->report_id[i], + cli_data->input_report[i], + cli_data->sensor_virt_addr[i]); + hid_input_report(cli_data->hid_sensor_hubs[i], + HID_INPUT_REPORT, + cli_data->input_report[i], report_size, 0); + } + + schedule_delayed_work(&cli_data->work_buffer, PERIOD); +}
...
quoted hunk ↗ jump to hunk
+static int amd_sfh_hid_client_probe(struct platform_device *pdev) +{ + struct amd_mp2_sensor_info info; + int rc = 0; + int i; + struct amdtp_cl_device *cl_dev; + struct amdtp_cl_data *cl_data; + u32 feature_report_size = 0; + u32 input_report_size = 0;
Are you sure all those assignments are needed?
+ cl_dev->pdev = pci_get_device(PCI_VENDOR_ID_AMD, + PCI_DEVICE_ID_AMD_MP2, NULL);
I'm not quite sure you understand what you are doing here. The code below is full of leak.
+ if (!cl_dev->pdev)
+ return -ENOMEM;
What? ...
+ cl_data = kzalloc(sizeof(*cl_data), GFP_KERNEL);
+ cl_data->num_hid_devices = amd_mp2_get_sensor_num + (cl_dev->pdev, &cl_data->sensor_idx[0]);
Strange indentation. NULL pointer dereference!
+ for (i = 0; i < cl_data->num_hid_devices; i++) {quoted hunk ↗ jump to hunk
+ cl_data->sensor_virt_addr[i] = dma_alloc_coherent + (&cl_dev->pdev->dev, sizeof(int) * 8, + &cl_data->sensor_phy_addr[i], GFP_KERNEL);
Indentation.
quoted hunk ↗ jump to hunk
+ cl_data->report_descr_size[i] = get_descriptor_size + (cl_data->sensor_idx[i], descr_size); + + feature_report_size = get_descriptor_size + (cl_data->sensor_idx[i], feature_size); + + input_report_size = get_descriptor_size + (cl_data->sensor_idx[i], input_size);
Ditto.
quoted hunk ↗ jump to hunk
+ cl_data->feature_report[i] = kzalloc(feature_report_size, + GFP_KERNEL); + cl_data->input_report[i] = kzalloc(input_report_size, + GFP_KERNEL);
NULL pointer dereference. You probably need to understand how memory allocation and heap usage works.
quoted hunk ↗ jump to hunk
+ info.period = PERIOD; + info.sensor_idx = cl_data->sensor_idx[i]; + info.phy_address = cl_data->sensor_phy_addr[i];
+ cl_data->report_descr[i] = kzalloc + (cl_data->report_descr_size[i], GFP_KERNEL);
Indentation.
quoted hunk ↗ jump to hunk
+ if (!cl_data->report_descr[i]) + return -ENOMEM; + rc = get_report_descriptor(cl_data->sensor_idx[i], + cl_data->report_descr[i]); + rc = amdtp_hid_probe(cl_data->cur_hid_dev, cl_data); + amd_start_sensor(cl_dev->pdev, info); + cl_data->sensor_sts[i] = 1; + } + + cl_dev->cl_data = cl_data; + cl_data_context = cl_data; + platform_set_drvdata(pdev, cl_dev); + schedule_delayed_work(&cl_data->work_buffer, PERIOD); + return 0; +}
...
quoted hunk ↗ jump to hunk
+static int amd_sfh_hid_client_remove(struct platform_device *pdev) +{ + int i; + struct amdtp_cl_device *cl_dev; + struct amdtp_cl_data *cl_data;
Reversed xmas tree, please.
+ cl_dev = platform_get_drvdata(pdev);
+ if (!(cl_dev)) + return -EINVAL;
When this will be true?! This can be done directly in the definition block above.
+ cl_data = cl_dev->cl_data;
+ if (!(cl_data)) + return -EINVAL;
Ditto.
quoted hunk ↗ jump to hunk
+ for (i = 0; i < cl_data->num_hid_devices; i++) + amd_stop_sensor(cl_dev->pdev, i); + + cancel_delayed_work_sync(&cl_data->work); + cancel_delayed_work_sync(&cl_data->work_buffer); + amdtp_hid_remove(cl_data); + + for (i = 0; i < cl_data->num_hid_devices; i++) { + if (cl_data->sensor_virt_addr[i]) + dma_free_coherent(&cl_dev->pdev->dev, 8 * sizeof(int), + cl_data->sensor_virt_addr[i], + cl_data->sensor_phy_addr[i]); + } +
+ kfree(cl_data);
Why not devm in the probe and nothing here?
+ pr_info("%s:%s Exit\n", DRIVER_NAME, __func__);No, no. Remove this noise.
+ return 0; +}
...
+static const struct acpi_device_id amd_sfh_acpi_match[] = { {
+ "AMDI0080" },+ { },No comma on terminators.
+};
Indentation is horrible. You have to look into existing drivers (better newer ones) to see how to have done things like this.
+
Blank line in wrong place...
+MODULE_DEVICE_TABLE(acpi, amd_sfh_acpi_match);
...should be here.
quoted hunk ↗ jump to hunk
+static struct platform_driver amd_sfh_plat_driver = { + .probe = amd_sfh_hid_client_probe, + .remove = amd_sfh_hid_client_remove, + .driver = { + .name = "amd_sfh_platform",
+ .acpi_match_table = ACPI_PTR(amd_sfh_acpi_match),
ACPI_PTR() is redundant and/or provokes compilation warnings.
quoted hunk ↗ jump to hunk
+ }, +}; +module_platform_driver(amd_sfh_plat_driver);
+ blank line. ...
quoted hunk ↗ jump to hunk
+static int amdtp_hid_parse(struct hid_device *hid) +{ + struct amdtp_hid_data *hid_data = hid->driver_data; + struct amdtp_cl_data *cli_data = hid_data->cli_data;
quoted hunk ↗ jump to hunk
+ int rc; + + rc = hid_parse_report(hid, cli_data->report_descr[hid_data->index], + cli_data->report_descr_size[hid_data->index]); + if (rc) + return rc; + return 0;
return hid_parse_report(...);
+}
...
quoted hunk ↗ jump to hunk
+static void amdtp_hid_request(struct hid_device *hid, struct hid_report *rep, + int reqtype) +{ + switch (reqtype) { + case HID_REQ_GET_REPORT: + amd_sfh_get_report(hid, rep->id, rep->type); + break; + case HID_REQ_SET_REPORT: + amd_sfh_set_report(hid, rep->id, reqtype); + break; + }
default?
+}
...
quoted hunk ↗ jump to hunk
+static int amdtp_wait_for_response(struct hid_device *hid) +{ + struct amdtp_hid_data *hid_data = hid->driver_data; + struct amdtp_cl_data *cli_data = hid_data->cli_data; + int i = 0; + + for (i = 0; i < cli_data->num_hid_devices; i++) { + if (cli_data->hid_sensor_hubs[i] == hid) + break; + } + if (!cli_data->request_done[i]) + wait_event_interruptible_timeout(hid_data->hid_wait, + cli_data->request_done[i], + 1500);
+ if (!cli_data->request_done[i]) {Shouldn't you rather check returned value from previous call?
quoted hunk ↗ jump to hunk
+ hid_err(hid, + "timeout waiting for response from device\n"); + return -ETIMEDOUT; + } + cli_data->request_done[i] = false; + + return 0; +}
...
+ hid = hid_allocate_device();
+ if (IS_ERR(hid)) {+ rc = PTR_ERR(hid); + return -ENOMEM;
Huh?!
+ }
...
+#ifndef AMDSFH_HID_H +#define AMDSFH_HID_H
+#include <linux/debugfs.h>
What?! I see useds of other stuff, but this... -- With Best Regards, Andy Shevchenko