Date:   Mon, 12 Sep 2016 13:57:22 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Tommy Lo <tommy.lo@...antech.com.tw>
Cc:     arnd@...db.de, linux-kernel@...r.kernel.org,
        oakley.ding@...antech.com.tw
Subject: Re: [PATCH] add Advantech cpci-1502 hard disk swap button driver

On Mon, Sep 12, 2016 at 09:42:20AM +0000, Tommy Lo wrote:
> 
> 
> 
> >Thanks for the patch, but can you resend it with a Signed-off-by: line, as described by Documentation/SubmittingPatches?  We need that before we can take anything.
> 
> >Also, you should run the code through scripts/checkpatch.pl and fix up the errors it finds.  There are still a number of basic coding style fixes that need to be done.
> 
> >And finally, this code really should tie the LED interaction into the LED kernel subsystem, so that you do not need a custom userspace program to handle ioctls just for LED control.  Do you need help in making those changes?
> 
> >thanks,
> 
> >greg k-h
> 
> Thanks for the information and advices sincerely. I ran the checkpatch.pl and eliminated most of the errors. 

Any reason you didn't fix the rest?

> The precise behavior of the interaction is bundled in the user application. While pushing the buttons it triggers a IQR to inform driver ,makes the CPLD LED blinking,
> and send the signal to make the app layer to umount the devices.
> 
> If it succeeds , it will turn off the LED , or it stops blinking and keeps the LED on.

So the userspace program is in charge here entirely?

That sounds odd.

> On the other hand , while plugging in the disk , it will try to verify the dev/devices wheather are shown in the file system. If succeeds,it will make the driver turn off the LED, or keep it on.
> 
> Because of this target , the behavior is  bumdled in the app partially. We also provide the user program for users to use directly or integrate into their systems.
> 
> This is the reason how it is designed. Could you please give me some iformation or advices to confirm if it's adequate ? 

Please use the in-kernel LED api for your LED control, and then change
your userspace application to use that api.  After that, we can look at
the rest of the user/kernel api you have here, but I really doubt you
will need a char device node for it.

Some comments on your code:




> 
> Thanks!
> Tommy
> 
> Signed-off-by: Tommy Lo <tommy.lo@...antech.com.tw>
> 
> 
> ---
>  drivers/char/Kconfig      |   9 +
>  drivers/char/Makefile     |   2 +
>  drivers/char/hdd_hp_btn.c | 632 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/char/hdd_hp_btn.h |  20 ++
>  4 files changed, 663 insertions(+)
>  create mode 100644 drivers/char/hdd_hp_btn.c
>  create mode 100644 drivers/char/hdd_hp_btn.h
> 
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index dcc0973..ca84ca0 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -590,5 +590,14 @@ config TILE_SROM
>  
>  source "drivers/char/xillybus/Kconfig"
>  
> +config HDD_HP_BTN
> +	tristate "Advantech CPCI-1502 hard disk hot swap button driver"
> +	default n
> +	help
> +	  This driver enables the support for the hard disk hot swap button driver.
> +	  ACPI6-B provide hot-plug buttons for each SAS HDD, and an interrupt (IRQ5) is generated when pressing button.
> +	  The module catches IRQ5,then raises signal to user space application after checking corresponding status register to get HDD status. 
> +	  It also provides ioctl method which response to CPLD to notify LED on/off.
> +
>  endmenu
>  
> diff --git a/drivers/char/Makefile b/drivers/char/Makefile
> index 6e6c244..ad45107 100644
> --- a/drivers/char/Makefile
> +++ b/drivers/char/Makefile
> @@ -60,3 +60,5 @@ js-rtc-y = rtc.o
>  obj-$(CONFIG_TILE_SROM)		+= tile-srom.o
>  obj-$(CONFIG_XILLYBUS)		+= xillybus/
>  obj-$(CONFIG_POWERNV_OP_PANEL)	+= powernv-op-panel.o
> +
> +obj-$(CONFIG_HDD_HP_BTN)	+= hdd_hp_btn.o
> \ No newline at end of file
> diff --git a/drivers/char/hdd_hp_btn.c b/drivers/char/hdd_hp_btn.c
> new file mode 100644
> index 0000000..9aa60be
> --- /dev/null
> +++ b/drivers/char/hdd_hp_btn.c
> @@ -0,0 +1,632 @@
> +/*
> +*  Advantech SAS hard disk hot swap button driver
> +*
> +*  Copyright (C) 2016 Advantech
> +*
> +*  This program is free software; you can redistribute it and/or modify
> +*  it under the terms of the GNU General Public License version 2 as
> +*  published by the Free Software Foundation.
> +*/
> +
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/init.h>
> +#include <linux/major.h>
> +#include <linux/fs.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/uaccess.h>
> +#include <linux/ioport.h>
> +#include <linux/io.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +
> +#include <asm/siginfo.h>
> +#include <linux/rcupdate.h>//rcu_read_lock
> +#include <linux/debugfs.h>
> +#include <linux/uaccess.h>
> +#include <linux/ioctl.h>
> +#include <linux/moduleparam.h>
> +#include "hdd_hp_btn.h"
> +
> +#define LPC_ADDR 0x900
> +#define EFB_WB_ADDR (LPC_ADDR+0x52)
> +#define EFB_WB_WRITE (LPC_ADDR+0x53)
> +#define EFB_WB_READ (LPC_ADDR+0x54)
> +#define EFB_WB_CTRL (LPC_ADDR+0x55)
> +#define EFB_WB_RD_CTL 0x2
> +#define EFB_WB_WR_CTL 0x1
> +
> +#define FRU_LED_ADDR (LPC_ADDR+0x16)
> +
> +#define FPGA_SIRQ_CFG (LPC_ADDR+0x2C)
> +#define FPGA_SIRQ_REG (LPC_ADDR+0x2E)
> +#define FPGA_SIRQ_5 0x1
> +#define FPGA_SIRQ_6 0x2
> +#define FPGA_SIRQ_7 0x3
> +#define FPGA_SIRQ_9 0x4
> +#define FPGA_SIRQ_10 0x5
> +#define FPGA_SIRQ_11 0x6
> +#define FPGA_SIRQ_12 0x7
> +#define FPGA_SIRQ_13 0x8
> +#define FPGA_SIRQ_14 0x9
> +#define FPGA_SIRQ_15 0xA
> +#define FPGA_SIRQ_INTA 0xC
> +#define FPGA_SIRQ_INTB 0xD
> +#define FPGA_SIRQ_INTC 0xE
> +#define FPGA_SIRQ_INTD 0xF
> +#define FPGA_SIRQ_NUM 15
> +
> +#define IOCTL_EFB_WB_WRITE 0x7F
> +#define IOCTL_EFB_WB_READ 0x7E
> +#define IOCTL_FRU_LED_CTL 0x80
> +/*************************************************************/
> +/*   Start of SYSFS kobject define                           */
> +/*                                                           */
> +
> +static struct kobject *lpc_kobj;
> +static struct kobject *lpc_user_led_kobj;
> +static struct kobject *lpc_register_kobj;
> +static struct kobject *lpc_rtm_kobj;

Never use "raw" kobjects in a driver, that's a huge sign that something
is wrong with it.  They should not be needed at all.

> +
> +static int free_irq_num = FPGA_SIRQ_NUM;
> +static int pid;
> +static int signal_num = HDD_SWAP_SIG;
> +
> +module_param(signal_num, int, 0);
> +
> +static ssize_t reg40_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	int reg;
> +
> +	reg = inb(LPC_ADDR + 0x40);
> +	return sprintf(buf, "%d:%02X\n", reg, reg);
> +}
> +
> +static ssize_t reg40_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	int reg;
> +
> +	if (sscanf(buf, "%d", &reg) != 1)
> +		return -EINVAL;
> +
> +	outb(reg, LPC_ADDR + 0x40);
> +	return count;
> +}
> +
> +static struct kobj_attribute reg40_attribute =
> +	__ATTR(40, 0664, reg40_show, reg40_store);

Not that you need this in the future, but please always use the
__ATTR_RW() and other forms of the macro.


> +
> +static ssize_t reg41_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	int reg;
> +
> +	reg = inb(LPC_ADDR + 0x41);
> +	return sprintf(buf, "%d:%02X\n", reg, reg);
> +}
> +
> +static ssize_t reg41_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	int reg;
> +
> +	if (sscanf(buf, "%d", &reg) != 1)
> +		return -EINVAL;
> +
> +	outb(reg, LPC_ADDR + 0x41);
> +	return count;
> +}
> +
> +static struct kobj_attribute reg41_attribute =
> +	__ATTR(41, 0664, reg41_show, reg41_store);
> +
> +static ssize_t reg42_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	int reg;
> +
> +	reg = inb(LPC_ADDR + 0x42);
> +	return sprintf(buf, "%d:%02X\n", reg, reg);
> +}
> +
> +static ssize_t reg42_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	int reg;
> +
> +	if (sscanf(buf, "%d", &reg) != 1)
> +		return -EINVAL;
> +
> +	outb(reg, LPC_ADDR + 0x42);
> +	return count;
> +}
> +
> +static struct kobj_attribute reg42_attribute =
> +	__ATTR(42, 0664, reg42_show, reg42_store);
> +
> +static ssize_t reg43_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	int reg;
> +
> +	reg = inb(LPC_ADDR + 0x43);
> +	return sprintf(buf, "%d:%02X\n", reg, reg);
> +}
> +
> +static ssize_t reg43_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	int reg;
> +
> +	if (sscanf(buf, "%d", &reg) != 1)
> +		return -EINVAL;
> +
> +	if (reg < 255 && reg >= 0)
> +		outb(reg, LPC_ADDR + 0x43);
> +	return count;
> +}
> +
> +static struct kobj_attribute reg43_attribute =
> +	__ATTR(43, 0664, reg43_show, reg43_store);
> +
> +static ssize_t reg30_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	int reg;
> +
> +	reg = inb(LPC_ADDR + 0x30);
> +	return sprintf(buf, "%d\n", reg);
> +}
> +
> +static ssize_t reg30_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	int reg;
> +
> +	if (sscanf(buf, "%d", &reg) != 1)
> +		return -EINVAL;
> +
> +	if (reg < 255 && reg >= 0)
> +		outb(reg, LPC_ADDR + 0x30);
> +	return count;
> +}
> +
> +static struct kobj_attribute reg30_attribute =
> +	__ATTR(30, 0664, reg30_show, reg30_store);
> +
> +static struct attribute *attrs_register[] = {
> +	&reg40_attribute.attr,
> +	&reg41_attribute.attr,
> +	&reg42_attribute.attr,
> +	&reg43_attribute.attr,
> +	&reg30_attribute.attr,
> +	NULL,
> +};

shouldn't all of this just be some debugfs files?  No userspace program
cares about these registers.

> +
> +static DECLARE_WAIT_QUEUE_HEAD(bt_wq0);
> +static int bt_wait0;
> +static int bt_flag0;

You only have support for one device in the system at a time?  not good.

> +/*******************************************************************/
> +/*       Start of DEVFS define                                     */

devfs went away over a decade ago, what code are you copying this from?


> +static ssize_t drv_read(struct file *filp, char *buf, size_t count, loff_t *ppos)
> +{
> +	return count;
> +}

why have read at all if you only act as a sink?

> +
> +static ssize_t drv_write(struct file *filp, const char *buf, size_t count, loff_t *ppos)
> +{
> +	char mybuf[10];
> +	/* read the value from user space */
> +	if (count > 10)
> +		return -EINVAL;

why 10?

> +	if (copy_from_user(mybuf, buf, count))
> +		return -EFAULT;
> +
> +	if (sscanf(mybuf, "%d", &pid) != 1)
> +		return -EFAULT;

what are you reading?



> +
> +	printk(KERN_INFO "User pid = %d\n", pid);

why does the pid end up in the kernel log?


> +
> +	return count;

So you just write a pid into the kernel log?  This seems really odd.


> +}
> +
> +static int drv_open(struct inode *inode, struct file *filp)
> +{
> +	return 0;

why is this needed?

> +}
> +
> +long drv_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +	struct ioctl_cmd data;
> +
> +	memset(&data, 0, sizeof(data));
> +
> +	switch (cmd) {
> +	case IOCTL_LED_ON:
> +		if (copy_from_user(&data, (int __user *) arg, sizeof(data)))
> +			return -EFAULT;
> +
> +		if (data.val == 1) {
> +			outb(0x10, LPC_ADDR + 0x43);
> +			printk(KERN_INFO"Dev:LED1 ON\n");
> +		} else if (data.val == 2) {
> +			outb(0x20, LPC_ADDR + 0x43);
> +			printk(KERN_INFO"Dev:LED2 ON\n");
> +		}
> +		break;
> +	case IOCTL_LED_OFF:
> +		if (copy_from_user(&data, (int __user *) arg, sizeof(data)))
> +			return -EFAULT;
> +
> +		if (data.val == 1) {
> +			outb(0x01, LPC_ADDR + 0x43);
> +			printk(KERN_INFO"Dev:LED1 OFF\n");
> +		} else if (data.val == 2) {
> +			outb(0x02, LPC_ADDR + 0x43);
> +			printk(KERN_INFO"Dev:LED2 OFF\n");
> +		}
> +		break;

Just use the LED api.

> +	}
> +	return 0;
> +}
> +
> +static int drv_release(struct inode *inode, struct file *filp)
> +{
> +	return 0;
> +}

again, not needed.

> +
> +irqreturn_t HDD_IRQ(int irq, void *dev_id)

odd function name.

> +{
> +	unsigned char tmp = 0;
> +
> +	printk(KERN_INFO"HDD_IRQ5:INTERRUPT\n");
> +	tmp = inb(LPC_ADDR + 0x40);
> +
> +	if ((tmp & 0x33) == 0)
> +		return IRQ_NONE;
> +
> +	outb(tmp, LPC_ADDR + 0x40);
> +
> +	/*HDD1_HP_SW*/
> +	if (tmp & 0x10) {
> +		send_signal(signal_num, SIG_BUTTON1_INVOKE);
> +		bt_flag0 = 1;
> +		wake_up_interruptible(&bt_wq0);
> +	}
> +
> +	/*HDD2_HP_SW*/
> +	if (tmp & 0x20) {
> +		send_signal(signal_num, SIG_BUTTON2_INVOKE);
> +		bt_flag1 = 1;
> +		wake_up_interruptible(&bt_wq1);
> +	}
> +
> +	/*HDD1_INSERT*/
> +	if (tmp & 0x01) {
> +		send_signal(signal_num, SIG_HDD1_INSERT);
> +		prsnt_flag0 = 1;
> +		wake_up_interruptible(&prsnt_wq0);
> +	}
> +
> +	/*HDD2_INSERT*/
> +	if (tmp & 0x02) {
> +		send_signal(signal_num, SIG_HDD2_INSERT);
> +		prsnt_flag1 = 1;
> +		wake_up_interruptible(&prsnt_wq1);
> +	}
> +
> +	return IRQ_HANDLED;
> +};
> +
> +const struct file_operations drv_fops = {
> +	owner: THIS_MODULE,
> +	read : drv_read,
> +	write : drv_write,
> +	unlocked_ioctl : drv_ioctl,
> +	open : drv_open,
> +	release : drv_release,
> +};
> +
> +#define DRIVER_NAME "hdd_hp_btn"
> +static unsigned int demo_chrdev_alloc_major = 0;
> +static unsigned int num_of_dev = 1;

why 1?

> +static struct cdev demo_chrdev_alloc_cdev;
> +struct class *demo_class;

Not that you really need it, but in the future, just use the misc_device
interface, much simpler and it does not burn a whole major number for
one minor device node.

thanks,

greg k-h

Powered by blists - more mailing lists