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

[Vladimir Stankovic <vladimir.stankovic@displaylink.com>]

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.