Re: [PATCH v6 1/8] usb: Add MA-USB Host kernel module

From: Vladimir Stankovic
Date: Thu Jun 11 2020 - 14:26:20 EST


On 15.5.20. 15:01, Greg KH wrote:
> On Fri, May 15, 2020 at 02:34:55PM +0200, Vladimir Stankovic wrote:
>> --- /dev/null
>> +++ b/drivers/usb/host/mausb/Kconfig
>> @@ -0,0 +1,15 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# Kernel configuration file for MA-USB Host driver.
>> +#
>> +# Copyright (c) 2019 - 2020 DisplayLink (UK) Ltd.
>> +#
>> +
>> +config USB_HOST_MAUSB
>> + tristate "Media Agnostic (MA) USB Host Driver"
>> + depends on USB=y
>
> Why =y? That should not be a requirement for any usb host driver.
With current driver in-tree location, it's not needed. Will be removed.
>
>> + help
>> + This is a Media Agnostic (MA) USB Host driver which enables host
>> + communication via MA USB protocol stack.
>> +
>> + If this driver is compiled as a module, it will be named mausb_host.
>
> Provide links to the userspace and spec here so that people have a
> chance to be able to use this driver?
>
>
>
>> diff --git a/drivers/usb/host/mausb/Makefile b/drivers/usb/host/mausb/Makefile
>> new file mode 100644
>> index 000000000000..cafccac0edba
>> --- /dev/null
>> +++ b/drivers/usb/host/mausb/Makefile
>> @@ -0,0 +1,10 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# Makefile for DisplayLink MA-USB Host driver.
>> +#
>> +# Copyright (c) 2019 - 2020 DisplayLink (UK) Ltd.
>> +#
>> +
>> +obj-$(CONFIG_USB_HOST_MAUSB) += mausb_host.o
>> +mausb_host-y := mausb_core.o
>> +mausb_host-y += utils.o
>> diff --git a/drivers/usb/host/mausb/mausb_core.c b/drivers/usb/host/mausb/mausb_core.c
>> new file mode 100644
>> index 000000000000..44f76a1b74de
>> --- /dev/null
>> +++ b/drivers/usb/host/mausb/mausb_core.c
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2019 - 2020 DisplayLink (UK) Ltd.
>> + */
>> +#include <linux/module.h>
>> +
>> +#include "utils.h"
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("DisplayLink (UK) Ltd.");
>> +
>> +static int mausb_host_init(void)
>> +{
>> + return mausb_host_dev_register();
>> +}
>> +
>> +static void mausb_host_exit(void)
>> +{
>> + dev_info(mausb_host_dev.this_device, "Module unloading started...");
>> + mausb_host_dev_deregister();
>> +}
>> +
>> +module_init(mausb_host_init);
>> +module_exit(mausb_host_exit);
>> diff --git a/drivers/usb/host/mausb/utils.c b/drivers/usb/host/mausb/utils.c
>> new file mode 100644
>> index 000000000000..1cfa2140311e
>> --- /dev/null
>> +++ b/drivers/usb/host/mausb/utils.c
>> @@ -0,0 +1,47 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2019 - 2020 DisplayLink (UK) Ltd.
>> + */
>> +#include "utils.h"
>> +
>> +#include <linux/fs.h>
>> +#include <linux/slab.h>
>> +
>> +#define MAUSB_KERNEL_DEV_NAME "mausb_host"
>> +#define MAUSB_READ_DEVICE_TIMEOUT_MS 500
>> +
>> +struct miscdevice mausb_host_dev;
>> +
>> +static int mausb_host_dev_open(struct inode *inode, struct file *filp)
>> +{
>> + filp->private_data = NULL;
>> +
>> + return 0;
>> +}
>> +
>> +static int mausb_host_dev_release(struct inode *inode, struct file *filp)
>> +{
>> + kfree(filp->private_data);
>> + filp->private_data = NULL;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct file_operations mausb_host_dev_fops = {
>> + .open = mausb_host_dev_open,
>> + .release = mausb_host_dev_release,
>> +};
>> +
>> +int mausb_host_dev_register(void)
>> +{
>> + mausb_host_dev.minor = MISC_DYNAMIC_MINOR;
>> + mausb_host_dev.name = MAUSB_KERNEL_DEV_NAME;
>> + mausb_host_dev.fops = &mausb_host_dev_fops;
>> + mausb_host_dev.mode = 0;
>
> You only have 1 device in the system at a time? With a global
> structure? And no locking at all?
>
> That feels _very_ wrong, why?
>
> And mode of 0? You don't want any userspace code to use this device
> node?
>
> confused,
>
> greg k-h
>
Agree. This will be revised in following patch version.


--
Regards,
Vladimir.