| 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: <20170619141322.devvmcbjaeq4jnv5@ninjato>
Date: Mon, 19 Jun 2017 16:13:22 +0200
From: Wolfram Sang <wsa@...-dreams.de>
To: Brendan Higgins <brendanhiggins@...gle.com>
Cc: robh+dt@...nel.org, mark.rutland@....com, tglx@...utronix.de,
jason@...edaemon.net, marc.zyngier@....com, joel@....id.au,
vz@...ia.com, mouse@...c.ru, clg@...d.org,
benh@...nel.crashing.org, ryan_chen@...eedtech.com,
linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org
Subject: Re: [PATCH v10 4/5] i2c: aspeed: added driver for Aspeed I2C
Hi Brendan,
here is my review. Only minor stuff, no real show-stopper.
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 144cbadc7c72..280f84a0d7d1 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -326,6 +326,16 @@ config I2C_POWERMAC
>
> comment "I2C system bus drivers (mostly embedded / system-on-chip)"
>
> +config I2C_ASPEED
> + tristate "Aspeed I2C Controller"
> + depends on ARCH_ASPEED
|| COMPILE_TEST?
> + help
> + If you say yes to this option, support will be included for the
> + Aspeed I2C controller.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-aspeed.
> +
> config I2C_AT91
> tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
> depends on ARCH_AT91
> +struct aspeed_i2c_bus {
> + struct i2c_adapter adap;
> + struct device *dev;
> + void __iomem *base;
> + /* Synchronizes I/O mem access to base. */
> + spinlock_t lock;
> + struct completion cmd_complete;
> + int irq;
'irq' not really needed, I'd think.
> + unsigned long parent_clk_frequency;
> + u32 bus_frequency;
> + /* Transaction state. */
> + enum aspeed_i2c_master_state master_state;
> + struct i2c_msg *msgs;
> + size_t buf_index;
> + size_t msgs_index;
> + size_t msgs_count;
> + bool send_stop;
> + int cmd_err;
> + /* Protected only by i2c_lock_bus */
> + int master_xfer_result;
> +};
...
> +static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
> +{
> + unsigned long time_left, flags;
> + int ret = 0;
> + u32 command;
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + command = readl(bus->base + ASPEED_I2C_CMD_REG);
> +
> + if (command & ASPEED_I2CD_SDA_LINE_STS) {
> + /* Bus is idle: no recovery needed. */
> + if (command & ASPEED_I2CD_SCL_LINE_STS)
> + goto out;
> + dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> + command);
> +
> + reinit_completion(&bus->cmd_complete);
> + writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_timeout(
> + &bus->cmd_complete, bus->adap.timeout);
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + if (time_left == 0)
> + goto reset_out;
> + else if (bus->cmd_err)
> + goto reset_out;
> + /* Recovery failed. */
> + else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
> + ASPEED_I2CD_SCL_LINE_STS))
> + goto reset_out;
> + /* Bus error. */
> + } else {
> + dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> + command);
Same dbg message as in the condition? Move it out of the 'if'?
> +
> + reinit_completion(&bus->cmd_complete);
> + writel(ASPEED_I2CD_BUS_RECOVER_CMD,
> + bus->base + ASPEED_I2C_CMD_REG);
Out of interest: What does the RECOVER_CMD do?
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_timeout(
> + &bus->cmd_complete, bus->adap.timeout);
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + if (time_left == 0)
> + goto reset_out;
> + else if (bus->cmd_err)
> + goto reset_out;
> + /* Recovery failed. */
> + else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
> + ASPEED_I2CD_SDA_LINE_STS))
> + goto reset_out;
> + }
> +
> +out:
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + return ret;
> +
> +reset_out:
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + return aspeed_i2c_reset(bus);
> +}
...
> + case ASPEED_I2C_MASTER_INACTIVE:
> + dev_err(bus->dev,
> + "master received interrupt 0x%08x, but is inactive",
> + irq_status);
> + bus->cmd_err = -EIO;
> + /* Do not STOP as we should be inactive. */
> + goto out_complete;
> + default:
> + WARN(1, "unknown master state\n");
> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + bus->cmd_err = -EIO;
> + goto out_complete;
> + }
> +error_and_stop:
> + bus->cmd_err = -EIO;
> + aspeed_i2c_do_stop(bus);
> + goto out_no_complete;
> +out_complete:
> + bus->msgs = NULL;
> + if (bus->cmd_err)
> + bus->master_xfer_result = bus->cmd_err;
> + else
> + bus->master_xfer_result = bus->msgs_index + 1;
> + complete(&bus->cmd_complete);
> +out_no_complete:
> + if (irq_status != status_ack)
> + dev_err(bus->dev,
> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> + irq_status, status_ack);
> + spin_unlock(&bus->lock);
> + return !!irq_status;
> +}
You return in the interrupt handler -EIO always in case of errors. Can
you check Documentation/i2c/fault-codes and see if you can follow those
guidelines? Especially ENXIO on NACK and EAGAIN...
> +
> +static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> +{
> + struct aspeed_i2c_bus *bus = dev_id;
> +
> + if (aspeed_i2c_master_irq(bus))
> + return IRQ_HANDLED;
> + else
> + return IRQ_NONE;
Ternary operator? Your choice, though...
And one more question: Why do you use adapter->algo_data instead of
i2c_{get|set}_adapdata?
Kind regards,
Wolfram
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists