Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 29 May 2020 17:20:35 +0300 | Subject | Re: [PATCH v5 3/4] SFH: Transport Driver to add support of AMD Sensor Fusion Hub (SFH |
| |
On Fri, May 29, 2020 at 4:43 PM Sandeep Singh <Sandeep.Singh@amd.com> wrote: > > From: Sandeep Singh <sandeep.singh@amd.com> > > 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 <sandeep.singh@amd.com> > Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
Use proper sequence and Co-developed-by if applicable.
> Reported-by: kbuild test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
What has been reported? Absence of the drivers?
...
> +#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?
> +#include "amdsfh-hid.h" > +#include "amd_mp2_pcie.h" > +#include "hid_descriptor/amd_sfh_hid_descriptor.h"
...
> +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!
> + 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);
...
> +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.
> + 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); > +}
...
> +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++) {
> + 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.
> + 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.
> + 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.
> + 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.
> + 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; > +}
...
> +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.
> + 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.
> +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.
> + }, > +}; > +module_platform_driver(amd_sfh_plat_driver);
+ blank line.
...
> +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;
> + 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(...);
> +}
...
> +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?
> +}
...
> +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?
> + 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
| |