drivers/fsi: Add SBEFIFO FSI client device driver
PATCH 1/1 drivers/fsi: Add SBEFIFO FSI client device driver
From: "Edward A. James" <[email protected]>
IBM POWER9 processors contain some embedded hardware and software bits collectively referred to as the self boot engine (SBE). One role of the SBE is to act as a proxy that provides access to the registers of the POWER chip from other (embedded) systems.
The POWER9 chip contains a hardware frontend for communicating with the SBE from remote systems called the SBEFIFO. The SBEFIFO logic is contained within an FSI CFAM and as such the driver implements an FSI bus device.
The SBE expects to communicate using a defined wire protocol; however, the driver knows nothing of the protocol and only provides raw access to the fifo device to userspace applications wishing to communicate with the SBE using the wire protocol.
The SBEFIFO consists of two hardware fifos. The upstream fifo is used by the driver to transfer data to the SBE on the POWER chip, from the system hosting the driver. The downstream fifo is used by the driver to transfer data from the SBE on the power chip to the system hosting the driver.
The driver also provides an in-kernel API for SBEFIFO client device drivers. In this way, applications and client drivers all have safe access to the hardware.
--- drivers/fsi/Kconfig | 7 + drivers/fsi/Makefile | 1 + drivers/fsi/fsi-sbefifo.c | 926 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/fsi-sbefifo.h | 30 ++ 4 files changed, 964 insertions(+) create mode 100644 drivers/fsi/fsi-sbefifo.c create mode 100644 include/linux/fsi-sbefifo.h
drivers/fsi/Kconfig +7 -0
@@ -33,6 +33,13 @@ config FSI_SCOM ---help--- This option enables an FSI based SCOM device driver. +config FSI_SBEFIFO + tristate "SBEFIFO FSI client device driver" + ---help--- + This option enables an FSI based SBEFIFO device driver. The SBEFIFO is + a pipe-like FSI device for communicating with the self boot engine + (SBE) on POWER processors. + endif endmenu
drivers/fsi/Makefile +1 -0
@@ -3,3 +3,4 @@ obj-$(CONFIG_FSI) += fsi-core.o obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o obj-$(CONFIG_FSI_SCOM) += fsi-scom.o +obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
drivers/fsi/fsi-sbefifo.c new file +926 -0
926 lines not on this page · raw message
include/linux/fsi-sbefifo.h new file +30 -0
@@ -0,0 +1,30 @@ +/* +* SBEFIFO FSI Client device driver +* +* Copyright (C) IBM Corporation 2017 +* +* 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. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERGCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef LINUX_FSI_SBEFIFO_H +#define LINUX_FSI_SBEFIFO_H + +struct device ; +struct sbefifo_client ; + +extern struct sbefifo_client *sbefifo_drv_open (struct device *dev, +unsigned long flags); +extern int sbefifo_drv_read (struct sbefifo_client *client,char *buf, +size_t len); +extern int sbefifo_drv_write (struct sbefifo_client *client,const char *buf, +size_t len); +extern void sbefifo_drv_release (struct sbefifo_client *client); + +#endif /* LINUX_FSI_SBEFIFO_H */
Signature · 1 line
1.8.3.1
Quoted 1 line · Edward A. James
+static
DEFINE_IDA(sbefifo_ida);
WHen your module exits, you don't clean up this structure. Common mistake :)
thanks,
greg k-h
Quoted 23 lines · Edward A. James
include/linux/fsi-sbefifo.h new file +20
-0@@ -0,0 +1,30 @@ +/* +* SBEFIFO FSI Client device driver +* +* Copyright (C) IBM Corporation 2017 +* +* 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. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERGCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef LINUX_FSI_SBEFIFO_H +#define LINUX_FSI_SBEFIFO_H + +struct device ; +struct sbefifo_client ;
include/linux/fsi-sbefifo.h line 30
Quoted 10 lines
include/linux/fsi-sbefifo.h new file +10
-0+ +extern struct sbefifo_client *sbefifo_drv_open (struct device *dev, +unsigned long flags); +extern int sbefifo_drv_read (struct sbefifo_client *client,char *buf, +size_t len); +extern int sbefifo_drv_write (struct sbefifo_client *client,const char *buf, +size_t len); +extern void sbefifo_drv_release (struct sbefifo_client *client); + +#endif /* LINUX_FSI_SBEFIFO_H */
Why do you need this .h file at all? Nothing is calling it, right?
thanks,
greg k-h
drivers/fsi/fsi-sbefifo.c line 800
Quoted 1 line · Edward A. James
drivers/fsi/fsi-sbefifo.c new file +1
-0+dev_info (dev,"Found sbefifo device\n" );
Don't be chatty, this isn't needed at all, right?
thanks,
greg k-h
drivers/fsi/fsi-sbefifo.c line 603
Quoted 6 lines · Edward A. James
drivers/fsi/fsi-sbefifo.c new file +6
-0+static ssize_t sbefifo_read (struct file *file,char __user *buf, +size_t len,loff_t *offset) +{ +struct sbefifo_client *client = file->private_data; + +WARN_ON (*offset);
Huh? Why?
drivers/fsi/fsi-sbefifo.c line 606
Quoted 2 lines
drivers/fsi/fsi-sbefifo.c new file +2
-0+if (!access_ok (VERIFY_WRITE , buf, len)) +return -EFAULT ;
What does this check? You should never need to call access_ok in any driver, copy_from/to_user handles that all for you just fine.
thanks,
greg k-h
drivers/fsi/fsi-sbefifo.c line 60
Quoted 13 lines · Edward A. James
drivers/fsi/fsi-sbefifo.c new file +13
-0+struct sbefifo { +struct timer_list poll_timer; +struct fsi_device *fsi_dev; +struct miscdevice mdev; +wait_queue_head_t wait; +struct list_head link; +struct list_head xfrs; +struct kref kref; +spinlock_t lock; +char name[32 ]; +int idx; +int rc; +};
You have a misc device, a pointer to a fsi_device, and a kref in this structure. Which one actually does the reference counting? It seems there are 3 different ways it could happen. That's not right at all, and ripe for lots and lots of confusion. Only use one please.
thanks,
greg k-h