| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <99b3f6a9-ab7d-b3dd-2a54-3fcd72607ec6@wanadoo.fr>
Date: Sun, 12 Feb 2023 09:02:59 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Tharun Kumar P <tharunkumar.pasumarthi@...rochip.com>,
linux-kernel@...r.kernel.org
Cc: linux-gpio@...r.kernel.org, gregkh@...uxfoundation.org,
arnd@...db.de, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add
OTP/EEPROM driver for the pci1xxxx switch
Le 12/02/2023 à 04:57, Tharun Kumar P a écrit :
> From: Kumaravel Thiagarajan <kumaravel.thiagarajan@...rochip.com>
>
> Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer, industrial,
> and automotive applications. This switch integrates OTP and EEPROM to enable
> customization of the part in the field. This patch provides the OTP/EEPROM
> driver to support the same.
>
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@...rochip.com>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@...rochip.com>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@...rochip.com>
Hi,
a few nits below, should some be useful.
CJ
> ---
> v4 -> v5:
> - Used proper errno
> - Removed un-necessary prints
>
> v3 -> v4:
> - Remove extra space, tab, un-necessary casting, paranthesis, do
> while(false) loops
> - Used read_poll_timeout for polling BUSY_BIT
>
> v2 -> v3:
> - Modified commit description to include build issues reported by Kernel
> test robot <lkp@...el.com> which are fixed in this patch
>
> v1 -> v2:
> - Resolve build issue reported by kernel test robot <lkp@...el.com>
> ---
> MAINTAINERS | 1 +
> drivers/misc/mchp_pci1xxxx/Kconfig | 1 +
> drivers/misc/mchp_pci1xxxx/Makefile | 2 +-
> .../misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c | 649 ++++++++++++++++++
> 4 files changed, 652 insertions(+), 1 deletion(-)
> create mode 100644 drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 69d1e8ad52c5..8a57683927fd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13803,6 +13803,7 @@ S: Supported
> F: drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> F: drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h
> F: drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c
> +F: drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
>
> MICROCHIP OTPC DRIVER
> M: Claudiu Beznea <claudiu.beznea@...rochip.com>
> diff --git a/drivers/misc/mchp_pci1xxxx/Kconfig b/drivers/misc/mchp_pci1xxxx/Kconfig
> index 4abb47de7219..67fa3299cfb6 100644
> --- a/drivers/misc/mchp_pci1xxxx/Kconfig
> +++ b/drivers/misc/mchp_pci1xxxx/Kconfig
> @@ -2,6 +2,7 @@ config GP_PCI1XXXX
> tristate "Microchip PCI1XXXX PCIe to GPIO Expander + OTP/EEPROM manager"
> depends on PCI
> depends on GPIOLIB
> + depends on BLOCK
> select GPIOLIB_IRQCHIP
> select AUXILIARY_BUS
> help
> diff --git a/drivers/misc/mchp_pci1xxxx/Makefile b/drivers/misc/mchp_pci1xxxx/Makefile
> index fc4615cfe28b..ae31251dab37 100644
> --- a/drivers/misc/mchp_pci1xxxx/Makefile
> +++ b/drivers/misc/mchp_pci1xxxx/Makefile
> @@ -1 +1 @@
> -obj-$(CONFIG_GP_PCI1XXXX) := mchp_pci1xxxx_gp.o mchp_pci1xxxx_gpio.o
> +obj-$(CONFIG_GP_PCI1XXXX) := mchp_pci1xxxx_gp.o mchp_pci1xxxx_gpio.o mchp_pci1xxxx_otpe2p.o
> diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> new file mode 100644
> index 000000000000..0d097afc84aa
> --- /dev/null
> +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> @@ -0,0 +1,649 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2022-2023 Microchip Technology Inc.
> +// PCI1xxxx OTP/EEPROM driver
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/bio.h>
> +#include <linux/blkdev.h>
> +#include <linux/blk-mq.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/iopoll.h>
> +#include <linux/kthread.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +
> +#include "mchp_pci1xxxx_gp.h"
> +
> +#define PERI_PF3_SYSTEM_REG_ADDR_BASE (0x2000)
> +#define PERI_PF3_SYSTEM_REG_LENGTH (0x4000)
> +
> +#define CONFIG_REG_ADDR_BASE (0)
> +#define EEPROM_REG_ADDR_BASE (0x0E00)
> +#define OTP_REG_ADDR_BASE (0x1000)
> +
> +#define MMAP_CFG_OFFSET(x) (CONFIG_REG_ADDR_BASE + (x))
> +
> +#define CFG_SYS_LOCK_OFFSET (0xA0)
> +#define CFG_SYS_LOCK_PF3 BIT(5)
> +
> +#define MMAP_OTP_OFFSET(x) (OTP_REG_ADDR_BASE + (x))
> +
> +#define OTP_PWR_DN_OFFSET (0x00)
> +#define OTP_ADDR_HIGH_OFFSET (0x04)
> +#define OTP_ADDR_LOW_OFFSET (0x08)
> +#define OTP_ADDR_BITS_OFFSET (0x0C)
> +#define OTP_PRGM_DATA_OFFSET (0x10)
> +#define OTP_PRGM_MODE_OFFSET (0x14)
> +#define OTP_RD_DATA_OFFSET (0x18)
> +#define OTP_FUNC_CMD_OFFSET (0x20)
> +#define OTP_TEST_CMD_OFFSET (0x24)
> +#define OTP_CMD_GO_OFFSET (0x28)
> +#define OTP_PASS_FAIL_OFFSET (0x2C)
> +#define OTP_STATUS_OFFSET (0x30)
> +#define OTP_MAX_PRG_OFFSET (0x34)
> +#define OTP_RSTB_PW_OFFSET (0x50)
> +#define OTP_PGM_PW_OFFSET (0x60)
> +#define OTP_READ_PW_OFFSET (0x70)
> +
> +#define OTP_PWR_DN_BIT BIT(0)
> +#define OTP_CMD_GO_BIT BIT(0)
> +#define OTP_PGM_MODE_BYTE_BIT BIT(0)
> +#define OTP_STATUS_BUSY_BIT BIT(0)
> +#define OTP_FUNC_PGM_BIT BIT(1)
> +#define OTP_FUNC_RD_BIT BIT(0)
> +
> +#define OTP_RW_TIMEOUT_MILLISECONDS (5)
> +#define OTP_STATUS_READ_DELAY_US (4000)
> +#define OTP_STATUS_READ_TIMEOUT_US (20000)
> +
> +#define MMAP_EEPROM_OFFSET(x) (EEPROM_REG_ADDR_BASE + (x))
> +
> +#define EEPROM_CMD_REG (0x00)
> +#define EEPROM_DATA_REG (0x04)
> +#define EEPROM_CFG_REG (0x08)
> +
> +#define EEPROM_CMD_EPC_BUSY_BIT BIT(31)
> +#define EEPROM_CMD_EPC_TIMEOUT_BIT BIT(17)
> +#define EEPROM_CMD_EPC_WRITE (BIT(29) | BIT(28))
> +
> +#define EEPROM_CFG_BAUD_RATE_100KHZ BIT(9)
> +#define EEPROM_CFG_SIZE_SEL BIT(12)
> +#define EEPROM_CFG_PULSE_WIDTH_100KHZ (BIT(17) | BIT(16))
> +#define OTP_EEPROM_SECTOR_SIZE (512)
> +#define OTP_SIZE_IN_BYTES (8 * 1024)
> +#define EEPROM_SIZE_IN_BYTES (8 * 1024)
> +
> +struct pci1xxxx_otp_e2p_device {
> + struct pci1xxxx_otp_e2p_disk *otp_e2p_device;
> + struct auxiliary_device *pdev;
> + void __iomem *reg_base;
> + int block_device_count;
> +};
> +
> +struct pci1xxxx_otp_e2p_disk {
> + struct blk_mq_tag_set tag_set;
> + struct auxiliary_device *pdev;
> + struct request_queue *queue;
> + struct mutex lock;
> + struct gendisk *gd;
> + bool has_eeprom;
> + int (*disk_write_byte)(struct pci1xxxx_otp_e2p_device *priv,
> + unsigned long byte_offset, u8 value);
> + int (*disk_read_byte)(struct pci1xxxx_otp_e2p_device *priv,
> + unsigned long byte_offset, u8 *data);
> +};
> +
> +static int otp_sector_count = OTP_SIZE_IN_BYTES / OTP_EEPROM_SECTOR_SIZE;
> +static int e2p_sector_count = EEPROM_SIZE_IN_BYTES / OTP_EEPROM_SECTOR_SIZE;
> +static int otp_device_count, e2p_device_count;
> +static int otp_block_driver_major;
> +
> +static void otp_device_set_address(struct pci1xxxx_otp_e2p_device *priv, u16 address)
> +{
> + u32 lo, hi;
> +
> + lo = address & 0xFF;
> + hi = (address & 0x1f00) >> 8;
> + writel(lo, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_LOW_OFFSET));
> + writel(hi, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_HIGH_OFFSET));
> +}
> +
> +static int set_sys_lock(struct pci1xxxx_otp_e2p_device *priv)
> +{
> + void __iomem *p = priv->reg_base + MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> + u8 data;
> +
> + writel(CFG_SYS_LOCK_PF3, p);
> + data = readl(p);
> + if (data != CFG_SYS_LOCK_PF3)
> + return -EPERM;
> +
> + return 0;
> +}
> +
> +static int release_sys_lock(struct pci1xxxx_otp_e2p_device *priv)
> +{
> + void __iomem *p = priv->reg_base + MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> + u8 data;
> +
> + data = readl(p);
> + if (data != CFG_SYS_LOCK_PF3)
> + return 0;
> +
> + writel(0, p);
> +
> + data = readl(p);
> + if (data & CFG_SYS_LOCK_PF3)
> + return -EPERM;
> +
> + return 0;
> +}
> +
> +static int otp_e2p_device_open(struct block_device *bdev, fmode_t mode)
> +{
> + struct pci1xxxx_otp_e2p_disk *disk_priv;
> + struct pci1xxxx_otp_e2p_device *priv;
> + struct auxiliary_device *pdev;
> + int retval = 0;
No need to initialize.
> + u8 data;
> +
> + disk_priv = bdev->bd_disk->private_data;
> + pdev = (struct auxiliary_device *)disk_priv->pdev;
> + priv = dev_get_drvdata(&pdev->dev);
> +
> + mutex_lock(&disk_priv->lock);
> +
> + retval = set_sys_lock(priv);
> + if (retval)
> + goto exit;
> +
> + if (!disk_priv->has_eeprom) {
> + data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> + writel((data & ~OTP_PWR_DN_BIT), priv->reg_base +
> + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> + }
> +
> +exit:
> + mutex_unlock(&disk_priv->lock);
> + return retval;
> +}
> +
> +static void otp_e2p_device_release(struct gendisk *disk, fmode_t mode)
> +{
> + struct pci1xxxx_otp_e2p_disk *disk_priv;
> + struct pci1xxxx_otp_e2p_device *priv;
> + u8 data;
> +
> + disk_priv = disk->private_data;
> + priv = dev_get_drvdata(&disk_priv->pdev->dev);
> +
> + mutex_lock(&disk_priv->lock);
> +
> + if (!disk_priv->has_eeprom) {
> + data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> + writel((data | OTP_PWR_DN_BIT), priv->reg_base +
> + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> + }
> + release_sys_lock(priv);
> +
> + mutex_unlock(&disk_priv->lock);
> +}
> +
> +static int e2p_device_write_byte(struct pci1xxxx_otp_e2p_device *priv,
> + unsigned long byte_offset, u8 value)
> +{
> + u32 data;
> +
> + /* Write the value into EEPROM_DATA_REG register */
> + writel(value, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_DATA_REG));
> + data = EEPROM_CMD_EPC_TIMEOUT_BIT | EEPROM_CMD_EPC_WRITE | byte_offset;
> +
> + /* Write the data into EEPROM_CMD_REG register */
> + writel(data, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> + /* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
> + writel(EEPROM_CMD_EPC_BUSY_BIT | data, priv->reg_base +
> + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> + /* Wait for the EPC_BUSY bit to get cleared */
> + do {
> + data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> + } while (data & EEPROM_CMD_EPC_BUSY_BIT);
Would it make sense to use read_poll_timeout() as below, instead of a
busy loop?
> +
> + if (data & EEPROM_CMD_EPC_TIMEOUT_BIT)
> + return -EBUSY;
> +
> + return 0;
> +}
> +
> +static int e2p_device_read_byte(struct pci1xxxx_otp_e2p_device *priv,
> + unsigned long byte_offset, u8 *data)
> +{
> + u32 regval;
> +
> + /*
> + * Write the byte offset into the EPC_ADDRESS field of EEPROM_CMD_REG
> + * register
> + */
> + writel(byte_offset, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> + /* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
> + writel(EEPROM_CMD_EPC_BUSY_BIT | byte_offset, priv->reg_base +
> + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> + /* Wait for the EPC_BUSY bit to get cleared */
> + do {
> + regval = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> + } while (regval & EEPROM_CMD_EPC_BUSY_BIT);
Would it make sense to use read_poll_timeout() as below, instead of a
busy loop?
> +
> + if (regval & EEPROM_CMD_EPC_TIMEOUT_BIT)
> + return -EBUSY;
> +
> + /* Read the contents from the EEPROM_DATA_REG */
> + *data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_DATA_REG));
> + return 0;
> +}
> +
> +static bool is_e2p_responsive(struct pci1xxxx_otp_e2p_device *priv)
> +{
> + u32 data;
> +
> + if (set_sys_lock(priv))
> + return false;
> +
> + writel((EEPROM_CFG_PULSE_WIDTH_100KHZ | EEPROM_CFG_SIZE_SEL |
> + EEPROM_CFG_BAUD_RATE_100KHZ), priv->reg_base +
> + MMAP_EEPROM_OFFSET(EEPROM_CFG_REG));
> +
> + /*
> + * Write the byte offset into the EPC_ADDRESS field of EEPROM_CMD_REG
> + * register
> + */
> + writel(EEPROM_CMD_EPC_TIMEOUT_BIT, priv->reg_base +
> + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> + /* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
> + writel(EEPROM_CMD_EPC_BUSY_BIT, priv->reg_base +
> + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> + /* Wait for the EPC_BUSY bit to get cleared or timeout bit to get set*/
> + do {
> + data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> + } while (data & EEPROM_CMD_EPC_BUSY_BIT);
Would it make sense to use read_poll_timeout() as below, instead of a
busy loop?
> +
> + /* If EPC_TIMEOUT is set, then the EEPROM is not responsive */
> + release_sys_lock(priv);
> +
> + if (data & EEPROM_CMD_EPC_TIMEOUT_BIT) {
> + dev_err(&priv->pdev->dev,
> + "EPC_Timeout, EEPROM is unresponsive: %x\n", data);
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static int otp_device_write_byte(struct pci1xxxx_otp_e2p_device *priv,
> + unsigned long byte_offset, u8 value)
> +{
> + u8 data;
> + int ret;
> +
> + if (!value)
> + return 0;
> +
> + otp_device_set_address(priv, (u16)byte_offset);
> +
> + /* Set OTP_PGM_MODE_BYTE command bit in OTP_PRGM_MODE register */
> + data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> + writel((data | OTP_PGM_MODE_BYTE_BIT), priv->reg_base +
> + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> +
> + /* Write the value to program into OTP_PRGM_DATA register */
> + writel(value, priv->reg_base + MMAP_OTP_OFFSET(OTP_PRGM_DATA_OFFSET));
> +
> + /* Set OTP_PROGRAM command bit in OTP_FUNC_CMD register */
> + data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> + writel((data | OTP_FUNC_PGM_BIT), priv->reg_base +
> + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +
> + /* Set OTP_GO command bit in OTP_CMD_GO register */
> + data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> + writel((data | OTP_CMD_GO_BIT), priv->reg_base +
> + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> + ret = read_poll_timeout(readl, data, !(data & OTP_STATUS_BUSY_BIT),
> + OTP_STATUS_READ_DELAY_US, OTP_STATUS_READ_TIMEOUT_US,
> + true, priv->reg_base + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
(here)
> + if (ret < 0)
> + return -EBUSY;
> +
> + /* Read the result from OTP_RD_DATA register */
> + data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
> + if (data & 0x02)
> + return 0;
> +
> + dev_err(&priv->pdev->dev, "OTP write read mismatch 0x%x\n", data);
> + return -EIO;
> +}
> +
> +static int otp_device_read_byte(struct pci1xxxx_otp_e2p_device *priv,
> + unsigned long byte_offset, u8 *data)
> +{
> + int ret;
Some places use ret, some other retval. (I personnaly prefer ret because
it is shorter)
Does it make sense to be consistent in the naming?
> +
> + otp_device_set_address(priv, (u16)byte_offset);
> +
> + /* Set OTP_READ command bit in OTP_FUNC_CMD register */
> + *data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> + writel((*data | OTP_FUNC_RD_BIT), priv->reg_base +
> + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +
> + /* Set OTP_GO command bit in OTP_CMD_GO register */
> + *data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> + writel((*data | OTP_CMD_GO_BIT), priv->reg_base +
> + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> + ret = read_poll_timeout(readl, *data, !(*data & OTP_STATUS_BUSY_BIT),
> + OTP_STATUS_READ_DELAY_US, OTP_STATUS_READ_TIMEOUT_US,
> + true, priv->reg_base + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> + if (ret < 0)
> + return -EBUSY;
> +
> + /* Read the result from OTP_RD_DATA register */
> + *data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_RD_DATA_OFFSET));
> + return 0;
> +}
> +
> +static int otp_e2P_device_transfer(struct request *req)
> +{
> + struct pci1xxxx_otp_e2p_disk *disk_priv;
> + struct pci1xxxx_otp_e2p_device *priv;
> + unsigned long sector;
> + unsigned long nsect;
> + long byte_offset;
> + int retval;
> + u8 *buffer;
> + int write;
> + int i, j;
> +
> + sector = blk_rq_pos(req);
> + nsect = blk_rq_cur_sectors(req);
> + buffer = bio_data(req->bio);
> + write = rq_data_dir(req);
> + disk_priv = req->q->disk->private_data;
> + priv = dev_get_drvdata(&disk_priv->pdev->dev);
> +
> + if (write) {
> + for (i = 0; i < nsect; i++) {
> + byte_offset = (sector + i) * OTP_EEPROM_SECTOR_SIZE;
> + for (j = 0; j < OTP_EEPROM_SECTOR_SIZE; j++) {
Not sure at all if it is a win, but testing for 'write' here would avoid
some code duplication.
> + retval = disk_priv->disk_write_byte(priv,
> + byte_offset + j,
> + *buffer);
> + if (retval)
> + return retval;
> +
> + buffer++;
> + }
> + }
> + } else {
> + for (i = 0; i < nsect; i++) {
> + byte_offset = (sector + i) * OTP_EEPROM_SECTOR_SIZE;
> + for (j = 0; j < OTP_EEPROM_SECTOR_SIZE; j++) {
> + retval = disk_priv->disk_read_byte(priv,
> + byte_offset + j,
> + buffer);
> + if (retval)
> + return retval;
> +
> + buffer++;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static blk_status_t OTPE2P_queue_rq(struct blk_mq_hw_ctx *hctx,
> + const struct blk_mq_queue_data *bd)
> +{
> + struct request *req = bd->rq;
> +
> + blk_mq_start_request(req);
> + if (!otp_e2P_device_transfer(req)) {
> + blk_mq_end_request(req, BLK_STS_OK);
> + return BLK_STS_OK;
> + }
> +
> + return BLK_STS_IOERR;
> +}
> +
> +static const struct blk_mq_ops OTPE2P_mq_ops = {
> + .queue_rq = OTPE2P_queue_rq,
> +};
> +
> +static const struct block_device_operations otp_e2p_device_ops = {
> + .owner = THIS_MODULE,
> + .open = otp_e2p_device_open,
> + .release = otp_e2p_device_release,
> +};
> +
> +static int otp_e2p_device_create_block_device(struct auxiliary_device *aux_dev)
> +{
> + struct auxiliary_device_wrapper *aux_dev_wrapper;
> + struct pci1xxxx_otp_e2p_device *priv;
> + struct gp_aux_data_type *pdata;
> + int retval = 0, i;
> +
> + aux_dev_wrapper = container_of(aux_dev, struct auxiliary_device_wrapper, aux_dev);
> + pdata = &aux_dev_wrapper->gp_aux_data;
> + if (!pdata) {
> + dev_err(&aux_dev->dev, "Invalid data in aux_dev_wrapper->gp_aux_data\n");
> + return -EINVAL;
> + }
> +
> + priv = devm_kzalloc(&aux_dev->dev, sizeof(struct pci1xxxx_otp_e2p_device),
> + GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->pdev = aux_dev;
> +
> + dev_set_drvdata(&aux_dev->dev, priv);
> +
> + if (!devm_request_mem_region(&aux_dev->dev, pdata->region_start +
> + PERI_PF3_SYSTEM_REG_ADDR_BASE,
> + PERI_PF3_SYSTEM_REG_LENGTH, aux_dev->name)) {
> + dev_err(&aux_dev->dev, "can't request otpe2p region\n");
> + return -ENOMEM;
> + }
> +
> + priv->reg_base = devm_ioremap(&aux_dev->dev, pdata->region_start +
> + PERI_PF3_SYSTEM_REG_ADDR_BASE,
> + PERI_PF3_SYSTEM_REG_LENGTH);
> + if (!priv->reg_base) {
> + dev_err(&aux_dev->dev, "ioremap failed\n");
> + return -ENOMEM;
> + }
> +
> + priv->block_device_count = 0;
Useless. It is zalloc()'ed and overwritten just below anyway.
> + if (is_e2p_responsive(priv))
> + priv->block_device_count = 2;
> + else
> + priv->block_device_count = 1;
> +
> + priv->otp_e2p_device = devm_kzalloc(&priv->pdev->dev,
> + priv->block_device_count *
> + sizeof(struct pci1xxxx_otp_e2p_disk),
> + GFP_KERNEL);
devm_kcalloc()?
> + if (!priv->otp_e2p_device)
> + return -ENOMEM;
> +
> + for (i = 0; i < priv->block_device_count; i++) {
having something like:
struct pci1xxxx_otp_e2p_disk *e2p_dev = &priv->otp_e2p_device[i];
would slighly simplify the code below and would help keeping statement
on 1 line.
> + mutex_init(&priv->otp_e2p_device[i].lock);
> + priv->otp_e2p_device[i].tag_set.ops = &OTPE2P_mq_ops;
> + priv->otp_e2p_device[i].tag_set.nr_hw_queues = 1;
> + priv->otp_e2p_device[i].tag_set.queue_depth = 16;
> + priv->otp_e2p_device[i].tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
> +
> + retval = blk_mq_alloc_tag_set(&priv->otp_e2p_device[i].tag_set);
> + if (retval) {
> + dev_err(&aux_dev->dev, "blk_mq_alloc_tag_set failed\n");
Should we have something like:
if (i)
goto otp_free_disk;
else
> + return retval;
> + }
> +
> + priv->otp_e2p_device[i].queue =
> + blk_mq_init_queue(&priv->otp_e2p_device[i].tag_set);
> + if (IS_ERR(priv->otp_e2p_device[i].queue)) {
> + retval = PTR_ERR(priv->otp_e2p_device[i].queue);
> + priv->otp_e2p_device[i].queue = NULL;
> + if (i)
> + goto e2p_free_tag_set;
> + else
> + goto otp_free_tag_set;
> + }
> +
> + blk_queue_logical_block_size(priv->otp_e2p_device[i].queue,
> + OTP_EEPROM_SECTOR_SIZE);
> + blk_queue_physical_block_size(priv->otp_e2p_device[i].queue,
> + OTP_EEPROM_SECTOR_SIZE);
> + priv->otp_e2p_device[i].queue->queuedata = priv;
> + priv->otp_e2p_device[i].gd =
> + blk_mq_alloc_disk(&priv->otp_e2p_device[i].tag_set,
> + NULL);
> + if (IS_ERR(priv->otp_e2p_device[i].gd)) {
> + retval = PTR_ERR(priv->otp_e2p_device[i].gd);
> + if (i)
> + goto e2p_destroy_queue;
> + else
> + goto otp_destroy_queue;
> + }
> +
> + priv->otp_e2p_device[i].pdev = aux_dev;
> + priv->otp_e2p_device[i].gd->major = otp_block_driver_major;
> + priv->otp_e2p_device[i].gd->minors = 1;
> + priv->otp_e2p_device[i].gd->first_minor =
> + otp_device_count + e2p_device_count;
> + priv->otp_e2p_device[i].gd->fops = &otp_e2p_device_ops;
> + priv->otp_e2p_device[i].gd->private_data =
> + &priv->otp_e2p_device[i];
> +
> + if (i == 0) {
> + snprintf(priv->otp_e2p_device[i].gd->disk_name,
> + 32, "PCI1xxxxOTP%x", otp_device_count);
s/32/DISK_NAME_LEN/ ?
> + set_capacity(priv->otp_e2p_device[i].gd, otp_sector_count);
> + priv->otp_e2p_device[i].disk_read_byte = otp_device_read_byte;
> + priv->otp_e2p_device[i].disk_write_byte = otp_device_write_byte;
> + otp_device_count++;
> + } else {
> + snprintf(priv->otp_e2p_device[i].gd->disk_name,
> + 32, "PCI1xxxxE2P%x", otp_device_count - 1);
s/32/DISK_NAME_LEN/ ?
> + set_capacity(priv->otp_e2p_device[i].gd, e2p_sector_count);
> + priv->otp_e2p_device[i].has_eeprom = true;
> + priv->otp_e2p_device[i].disk_read_byte = e2p_device_read_byte;
> + priv->otp_e2p_device[i].disk_write_byte = e2p_device_write_byte;
> + e2p_device_count++;
> + }
> +
> + retval = add_disk(priv->otp_e2p_device[i].gd);
> + if (retval) {
> + if (i)
> + goto e2p_free_disk;
> + else
> + goto otp_free_disk;
> + }
> + }
> +
> + return retval;
> +
> +e2p_free_disk:
> + del_gendisk(priv->otp_e2p_device[1].gd);
> + put_disk(priv->otp_e2p_device[1].gd);
> +e2p_destroy_queue:
> + blk_mq_destroy_queue(priv->otp_e2p_device[1].queue);
> +e2p_free_tag_set:
> + blk_mq_free_tag_set(&priv->otp_e2p_device[1].tag_set);
> +otp_free_disk:
> + del_gendisk(priv->otp_e2p_device[0].gd);
> + put_disk(priv->otp_e2p_device[0].gd);
> +otp_destroy_queue:
> + blk_mq_destroy_queue(priv->otp_e2p_device[0].queue);
> +otp_free_tag_set:
> + blk_mq_free_tag_set(&priv->otp_e2p_device[0].tag_set);
> +
> + dev_err(&aux_dev->dev,
> + "otp/eeprom device enumeration failed with errno = %d\n",
> + retval);
Maybe dev_err_probe() here?
(and/or even in other places above?)
> + return retval;
> +}
> +
> +static void pci1xxxx_otp_e2p_remove(struct auxiliary_device *aux_dev)
> +{
> + struct pci1xxxx_otp_e2p_device *priv = dev_get_drvdata(&aux_dev->dev);
> + int i;
> +
> + for (i = 0; i < priv->block_device_count; i++) {
> + if (priv->otp_e2p_device[i].queue)
> + blk_mq_destroy_queue(priv->otp_e2p_device[i].queue);
> +
> + if (priv->otp_e2p_device[i].gd) {
> + del_gendisk(priv->otp_e2p_device[i].gd);
> + put_disk(priv->otp_e2p_device[i].gd);
> + blk_mq_free_tag_set(&priv->otp_e2p_device[i].tag_set);
> + }
> + }
> +}
> +
> +static int pci1xxxx_otp_e2p_probe(struct auxiliary_device *aux_dev,
> + const struct auxiliary_device_id *id)
> +{
> + return otp_e2p_device_create_block_device(aux_dev);
> +}
> +
> +static const struct auxiliary_device_id pci1xxxx_otp_e2p_auxiliary_id_table[] = {
> + {.name = "mchp_pci1xxxx_gp.gp_otp_e2p"},
> + {},
> +};
> +MODULE_DEVICE_TABLE(auxiliary, pci1xxxx_otp_e2p_auxiliary_id_table);
> +
> +static struct auxiliary_driver pci1xxxx_otp_e2p_driver = {
> + .driver = {
> + .name = "PCI1xxxxOTPE2P",
> + },
> + .probe = pci1xxxx_otp_e2p_probe,
> + .remove = pci1xxxx_otp_e2p_remove,
> + .id_table = pci1xxxx_otp_e2p_auxiliary_id_table
> +};
> +
> +static int __init pci1xxxx_otp_e2p_driver_init(void)
> +{
> + int retval;
> +
> + otp_block_driver_major = register_blkdev(otp_block_driver_major,
> + "OTPBlockDevice");
Would it make sense to have a #define for "OTPBlockDevice"?
> + if (otp_block_driver_major < 0)
> + return otp_block_driver_major;
> +
> + retval = auxiliary_driver_register(&pci1xxxx_otp_e2p_driver);
> + if (retval)
> + unregister_blkdev(otp_block_driver_major, "OTPBlockDevice");
> +
> + return retval;
> +}
> +
> +static void __exit pci1xxxx_otp_e2p_driver_exit(void)
> +{
> + unregister_blkdev(otp_block_driver_major, "OTPBlockDevice");
> + auxiliary_driver_unregister(&pci1xxxx_otp_e2p_driver);
I think it is harmless, but shouldn't it be done in the reverse order to
match how resources have been allocated?
> +}
> +
> +module_init(pci1xxxx_otp_e2p_driver_init);
> +module_exit(pci1xxxx_otp_e2p_driver_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Kumaravel Thiagarajan <kumaravel.thiagarajan@...rochip.com>");
> +MODULE_AUTHOR("Tharun Kumar P<tharunkumar.pasumarthi@...rochip.com>");
> +MODULE_DESCRIPTION("Microchip Technology Inc. PCI1xxxx OTP EEPROM programmer");
Powered by blists - more mailing lists