| 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
| ||
|
Date: Fri, 22 May 2020 16:45:47 +0200
From: Wolfram Sang <wsa@...-dreams.de>
To: Tali Perry <tali.perry1@...il.com>
Cc: ofery@...gle.com, brendanhiggins@...gle.com,
avifishman70@...il.com, tmaimon77@...il.com, kfting@...oton.com,
venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com,
robh+dt@...nel.org, andriy.shevchenko@...ux.intel.com,
linux-arm-kernel@...ts.infradead.org, linux-i2c@...r.kernel.org,
openbmc@...ts.ozlabs.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller
driver
On Thu, May 21, 2020 at 02:09:09PM +0300, Tali Perry wrote:
> Add Nuvoton NPCM BMC I2C controller driver.
>
> Signed-off-by: Tali Perry <tali.perry1@...il.com>
This is a very complex driver, so I can really comment only about high
level things. Thank you very much for keeping at it!
My code checkers say:
CHECKPATCH:
CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.rst
#1210: FILE: drivers/i2c/busses/i2c-npcm7xx.c:1161:
+ udelay(200);
(a few of them)
GCC:
CC drivers/i2c/busses/i2c-npcm7xx.o
drivers/i2c/busses/i2c-npcm7xx.c: In function ‘npcm_i2c_reset’:
drivers/i2c/busses/i2c-npcm7xx.c:521:5: warning: variable ‘i2cctl2’ set but not used [-Wunused-but-set-variable]
> +/* Status of one I2C module */
> +struct npcm_i2c {
> + struct i2c_adapter adap;
> + struct device *dev;
> + unsigned char __iomem *reg;
> + spinlock_t lock; /* IRQ synchronization */
> + struct completion cmd_complete;
> + int irq;
> + int cmd_err;
> + struct i2c_msg *msgs;
> + int msgs_num;
> + int num;
> + u32 apb_clk;
> + struct i2c_bus_recovery_info rinfo;
> + enum i2c_state state;
> + enum i2c_oper operation;
> + enum i2c_mode master_or_slave;
> + enum i2c_state_ind stop_ind;
> + u8 dest_addr;
> + u8 *rd_buf;
> + u16 rd_size;
> + u16 rd_ind;
> + u8 *wr_buf;
> + u16 wr_size;
> + u16 wr_ind;
> + bool fifo_use;
> + u16 PEC_mask; /* PEC bit mask per slave address */
> + bool PEC_use;
> + bool read_block_use;
> + u8 int_cnt;
What is this for? It is written to but never read.
> + u32 clk_period_us;
Not used? Seems this struct could need some cleaning up.
> + unsigned long int_time_stamp;
> + unsigned long bus_freq; /* in kHz */
> + u32 xmits;
> +#ifdef CONFIG_DEBUG_FS
> + struct dentry *debugfs; /* debugfs device directory */
> + u64 ber_cnt;
> + u64 rec_succ_cnt;
> + u64 rec_fail_cnt;
> + u64 nack_cnt;
> + u64 timeout_cnt;
> +#endif
> +};
> +
...
> +static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)
> +{
> + if (bus->operation == I2C_READ_OPER)
> + return bus->rd_ind;
> + if (bus->operation == I2C_WRITE_OPER)
> + return bus->wr_ind;
> + return 0;
I2C_NO_OPER?
...
> +/* recovery using bit banging functionality of the module */
> +static int npcm_i2c_recovery_init(struct i2c_adapter *_adap)
> +{
> + struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap);
> + struct i2c_bus_recovery_info *rinfo = &bus->rinfo;
> +
> + rinfo->recover_bus = npcm_i2c_recovery_tgclk;
> + rinfo->prepare_recovery = NULL;
> + rinfo->unprepare_recovery = NULL;
> + rinfo->set_scl = NULL;
> + rinfo->set_sda = NULL;
'bus' is kzalloced, so no need for these NULLs.
What I wonder more, though, is if you can't populate {set|get}_{scl|sda}
and use the internal i2c_generic_scl_recovery()? Are there any issues
with it?
> +
> + dev_dbg(bus->dev, "init i2c recovery using TGCLK\n");
There is no error path here, so I think this message is not useful.
Means also this function could be 'void'.
> +
> + rinfo->get_scl = npcm_i2c_get_SCL;
> + rinfo->get_sda = npcm_i2c_get_SDA;
Not needed when you have a custom function.
> +
> + _adap->bus_recovery_info = rinfo;
> +
> + return 0;
> +}
> +
...
> +static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
> + struct npcm_i2c *bus = container_of(adap, struct npcm_i2c, adap);
> + struct i2c_msg *msg0, *msg1;
> + unsigned long time_left, flags;
> + u16 nwrite, nread;
> + u8 *write_data, *read_data;
> + u8 slave_addr;
> + int timeout;
> + int ret = 0;
> + bool read_block = false;
> + bool read_PEC = false;
> + u8 bus_busy;
> + unsigned long timeout_usec;
> +
> + if (bus->state == I2C_DISABLE) {
> + dev_err(bus->dev, "I2C%d module is disabled", bus->num);
> + return -EINVAL;
> + }
> +
> + if (num > 2 || num < 1) {
> + dev_err(bus->dev, "I2C cmd not supported num of msgs=%d", num);
> + return -EINVAL;
> + }
Since you have an 'i2c_adapter_quirks' struct filled, the core will
I2C check that for you.
> +
> + msg0 = &msgs[0];
> + slave_addr = msg0->addr;
> + if (msg0->flags & I2C_M_RD) { /* read */
> + if (num == 2) {
> + dev_err(bus->dev, "num=2 but 1st msg rd instead of wr");
> + return -EINVAL;
Ditto.
> + }
> + nwrite = 0;
> + write_data = NULL;
> + read_data = msg0->buf;
> + if (msg0->flags & I2C_M_RECV_LEN) {
> + nread = 1;
> + read_block = true;
> + if (msg0->flags & I2C_CLIENT_PEC)
> + read_PEC = true;
> + } else {
> + nread = msg0->len;
> + }
> + } else { /* write */
> + nwrite = msg0->len;
> + write_data = msg0->buf;
> + nread = 0;
> + read_data = NULL;
> + if (num == 2) {
> + msg1 = &msgs[1];
> + read_data = msg1->buf;
> + if (slave_addr != msg1->addr) {
> + dev_err(bus->dev,
> + "SA==%02x but msg1->addr==%02x\n",
> + slave_addr, msg1->addr);
> + return -EINVAL;
Ditto.
> + }
> + if ((msg1->flags & I2C_M_RD) == 0) {
> + dev_err(bus->dev,
> + "num = 2 but both msg are write.\n");
> + return -EINVAL;
> + }
Ditto.
> + if (msg1->flags & I2C_M_RECV_LEN) {
> + nread = 1;
> + read_block = true;
> + if (msg1->flags & I2C_CLIENT_PEC)
> + read_PEC = true;
> + } else {
> + nread = msg1->len;
> + read_block = false;
> + }
> + }
> + }
> +
> + /* Adaptive TimeOut: astimated time in usec + 100% margin */
> + timeout_usec = (2 * 10000 / bus->bus_freq) * (2 + nread + nwrite);
> + timeout = max(msecs_to_jiffies(35), usecs_to_jiffies(timeout_usec));
> + if (nwrite >= 32 * 1024 || nread >= 32 * 1024) {
> + dev_err(bus->dev, "i2c%d buffer too big\n", bus->num);
> + return -EINVAL;
> + }
Ditto.
> +
> + time_left = jiffies + msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;
> + do {
> + /*
> + * we must clear slave address immediately when the bus is not
> + * busy, so we spinlock it, but we don't keep the lock for the
> + * entire while since it is too long.
> + */
> + spin_lock_irqsave(&bus->lock, flags);
> + bus_busy = ioread8(bus->reg + NPCM_I2CCST) & NPCM_I2CCST_BB;
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + } while (time_is_after_jiffies(time_left) && bus_busy);
> +
> + if (bus_busy) {
> + iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
> + npcm_i2c_reset(bus);
> + i2c_recover_bus(adap);
> + return -EAGAIN;
> + }
> +
> + npcm_i2c_init_params(bus);
> + bus->dest_addr = slave_addr;
> + bus->msgs = msgs;
> + bus->msgs_num = num;
> + bus->cmd_err = 0;
> + bus->read_block_use = read_block;
> +
> + reinit_completion(&bus->cmd_complete);
> + if (!npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
> + write_data, read_data, read_PEC,
> + read_block))
> + ret = -EBUSY;
> +
> + if (ret != -EBUSY) {
> + time_left = wait_for_completion_timeout(&bus->cmd_complete,
> + timeout);
> +
> + if (time_left == 0) {
> +#ifdef CONFIG_DEBUG_FS
> + if (bus->timeout_cnt == ULLONG_MAX) {
> + dev_dbg(bus->dev,
> + "timeout_cnt reach max, reset to 0");
> + bus->timeout_cnt = 0;
> + }
> + bus->timeout_cnt++;
> +#endif
> + if (bus->master_or_slave == I2C_MASTER) {
> + i2c_recover_bus(adap);
> + bus->cmd_err = -EIO;
> + bus->state = I2C_IDLE;
> + }
> + }
> + }
> + ret = bus->cmd_err;
> +
> + /* if there was BER, check if need to recover the bus: */
> + if (bus->cmd_err == -EAGAIN)
> + ret = i2c_recover_bus(adap);
> +
> + return bus->cmd_err;
> +}
> +
> +static u32 npcm_i2c_functionality(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_EMUL |
> + I2C_FUNC_SMBUS_BLOCK_DATA |
> + I2C_FUNC_SMBUS_PEC |
> + I2C_FUNC_SLAVE;
> +}
> +
> +static const struct i2c_adapter_quirks npcm_i2c_quirks = {
> + .max_read_len = 32768,
> + .max_write_len = 32768,
These limits are for simple reads/writes with num_msgs == 1. If you have
limits also for num_msgs == 2, then you also need to fill
'max_comb_1st_msg_len' and 'max_comb_2nd_msg_len'. (Because for some HW
these are different values then)
> + .max_num_msgs = 2,
You can drop this because I2C_AQ_COMB_WRITE_THEN_READ implies it.
> + .flags = I2C_AQ_COMB_WRITE_THEN_READ,
> +};
> +
> +static const struct i2c_algorithm npcm_i2c_algo = {
> + .master_xfer = npcm_i2c_master_xfer,
> + .functionality = npcm_i2c_functionality,
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +/* i2c debugfs directory: used to keep health monitor of i2c devices */
> +static struct dentry *npcm_i2c_debugfs_dir;
> +
> +static void i2c_init_debugfs(struct platform_device *pdev, struct npcm_i2c *bus)
> +{
> + struct dentry *d;
> +
> + if (!npcm_i2c_debugfs_dir)
> + return;
> +
> + d = debugfs_create_dir(dev_name(&pdev->dev), npcm_i2c_debugfs_dir);
> + if (IS_ERR_OR_NULL(d))
> + return;
> +
> + debugfs_create_u64("ber_cnt", 0444, d, &bus->ber_cnt);
> + debugfs_create_u64("nack_cnt", 0444, d, &bus->nack_cnt);
> + debugfs_create_u64("rec_succ_cnt", 0444, d, &bus->rec_succ_cnt);
> + debugfs_create_u64("rec_fail_cnt", 0444, d, &bus->rec_fail_cnt);
> + debugfs_create_u64("timeout_cnt", 0444, d, &bus->timeout_cnt);
> +
> + bus->debugfs = d;
> +}
> +#else
> +static void i2c_init_debugfs(struct platform_device *pdev, struct npcm_i2c *bus)
> +{
> +}
> +#endif
> +
> +static int npcm_i2c_probe_bus(struct platform_device *pdev)
> +{
> + struct npcm_i2c *bus;
> + struct i2c_adapter *adap;
> + struct clk *i2c_clk;
> + static struct regmap *gcr_regmap;
> + static struct regmap *clk_regmap;
> + int ret;
> + int num;
> +
> + bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> + if (!bus)
> + return -ENOMEM;
> +
> + bus->dev = &pdev->dev;
> +
> + num = of_alias_get_id(pdev->dev.of_node, "i2c");
> + bus->num = num;
Why not assigning it directly and save the 'num' variable?
> + /* core clk must be acquired to calculate module timing settings */
> + i2c_clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(i2c_clk))
> + return PTR_ERR(i2c_clk);
> + bus->apb_clk = clk_get_rate(i2c_clk);
> +
> + gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
> + if (IS_ERR(gcr_regmap))
> + return IS_ERR(gcr_regmap);
> + regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
> +
> + clk_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-clk");
> + if (IS_ERR(clk_regmap))
> + return IS_ERR(clk_regmap);
> +
> + bus->reg = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(bus->reg))
> + return PTR_ERR((bus)->reg);
> +
> + spin_lock_init(&bus->lock);
> + init_completion(&bus->cmd_complete);
> +
> + adap = &bus->adap;
> + adap->owner = THIS_MODULE;
> + adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD | I2C_CLIENT_SLAVE;
Since you have a DT compatible, you won't need classes. Just drop them.
> + adap->retries = 3;
> + adap->timeout = HZ;
> + adap->algo = &npcm_i2c_algo;
> + adap->quirks = &npcm_i2c_quirks;
> + adap->algo_data = bus;
> + adap->dev.parent = &pdev->dev;
> + adap->dev.of_node = pdev->dev.of_node;
> + adap->nr = pdev->id;
> +
> + bus->irq = platform_get_irq(pdev, 0);
> + if (bus->irq < 0)
> + return bus->irq;
> +
> + ret = devm_request_irq(bus->dev, bus->irq, npcm_i2c_bus_irq, 0,
> + dev_name(bus->dev), bus);
> + if (ret)
> + return ret;
> +
> + ret = __npcm_i2c_init(bus, pdev);
> + if (ret)
> + return ret;
> +
> + ret = npcm_i2c_recovery_init(adap);
> + if (ret)
> + return ret;
> +
> + i2c_set_adapdata(adap, bus);
> +
> + snprintf(bus->adap.name, sizeof(bus->adap.name), "Nuvoton i2c");
Maybe you want to add something more specific in case you have multiple
instances of this driver at runtime.
> + ret = i2c_add_numbered_adapter(&bus->adap);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to add numbered adapter %d\n", ret);
The I2C core will print warnings for you.
> + return ret;
> + }
> + platform_set_drvdata(pdev, bus);
> +
> + i2c_init_debugfs(pdev, bus);
> + return 0;
> +}
> +
...
> +#ifdef CONFIG_DEBUG_FS
> +static int __init npcm_i2c_init(void)
> +{
> + struct dentry *dir;
> +
> + dir = debugfs_create_dir("i2c", NULL);
Okay, the GPIO fault injector could also need such a directory. I will
add this to the core. And then send an incremental patch for your
driver.
> + if (IS_ERR_OR_NULL(dir))
> + return 0;
> +
> + npcm_i2c_debugfs_dir = dir;
> + return 0;
> +}
> +
> +static void __exit npcm_i2c_exit(void)
> +{
> + debugfs_remove_recursive(npcm_i2c_debugfs_dir);
> +}
> +
> +module_init(npcm_i2c_init);
> +module_exit(npcm_i2c_exit);
> +#endif
> +
> +MODULE_AUTHOR("Avi Fishman <avi.fishman@...il.com>");
> +MODULE_AUTHOR("Tali Perry <tali.perry@...oton.com>");
> +MODULE_AUTHOR("Tyrone Ting <kfting@...oton.com>");
> +MODULE_DESCRIPTION("Nuvoton I2C Bus Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION("0.1.3");
> +
> --
> 2.22.0
>
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists