| 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: <9a438ecd4ac48d9546d76058e615419bb0942b42.camel@ew.tq-group.com>
Date: Mon, 20 Oct 2025 10:37:00 +0200
From: Matthias Schiffer <matthias.schiffer@...tq-group.com>
To: Andi Shyti <andi.shyti@...nel.org>
Cc: Lee Jones <lee@...nel.org>, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org, linux@...tq-group.com
Subject: Re: [PATCH v2 1/4] i2c: machxo2: new driver
On Sat, 2025-10-18 at 01:24 +0200, Andi Shyti wrote:
> Hi Matthias,
>
> ...
>
> > +enum machxo2_i2c_state {
> > + STATE_DONE,
> > + STATE_START,
> > + STATE_WRITE,
> > + STATE_READ_WAIT_SRW,
> > + STATE_READ,
>
> Can you please use a prefix for these enums?
>
> > +};
>
> ...
>
> > +/* Registers */
> > +#define I2C_CR 0 /* Control, RW */
> > +#define I2C_CR_I2CEN 0x80 /* Enable I2C */
> > +
> > +#define I2C_CMDR 1 /* Command, RW */
> > +#define I2C_CMDR_STA 0x80 /* Start */
> > +#define I2C_CMDR_STO 0x40 /* Stop */
> > +#define I2C_CMDR_RD 0x20 /* Read */
> > +#define I2C_CMDR_WR 0x10 /* Write */
> > +#define I2C_CMDR_ACK 0x08 /* Send NACK if the bit is set */
> > +#define I2C_CMDR_CKSDIS 0x04 /* Clock stretch disable */
> > +
> > +#define I2C_BR0 2 /* Clock Pre-scale, RW */
> > +#define I2C_BR1 3 /* Clock Pre-scale, RW */
> > +#define I2C_TXDR 4 /* Transmit Data, WO */
> > +#define I2C_SR 5 /* Status, RO */
> > +#define I2C_SR_TIP 0x80 /* Transmit In Progress */
> > +#define I2C_SR_BUSY 0x40 /* Bus Busy */
> > +#define I2C_SR_RARC 0x20 /* Received ACK (if unset) */
> > +#define I2C_SR_SRW 0x10 /* Slave Read/Write */
> > +#define I2C_SR_ARBL 0x08 /* Arbitration Lost */
> > +#define I2C_SR_TRRDY 0x04 /* Transmitter/Receiver Ready */
> > +#define I2C_SR_TROE 0x02 /* Transmitter/Receiver Overrun Error */
> > +#define I2C_SR_HGC 0x01 /* Hardware General Call Received */
> > +#define I2C_RXDR 7 /* Receive Data, RO */
> > +#define I2C_IRQ 8 /* IRQ, RW */
> > +#define I2C_IRQEN 9 /* IRQ Enable, RW */
> > +#define I2C_IRQ_ARBL 0x08 /* Arbitration Lost */
> > +#define I2C_IRQ_TRRDY 0x04 /* Transmitter/Receiver Ready */
> > +#define I2C_IRQ_TROE 0x02 /* Transmitter/Receiver Overrun Error */
> > +#define I2C_IRQ_HGC 0x01 /* Hardware General Call Received */
> > +#define I2C_IRQ_MASK (I2C_IRQ_ARBL | I2C_IRQ_TRRDY | I2C_IRQ_TROE)
>
> Can you please use a prefix for all these defines?
>
> ...
>
> > +/* Delay by a number of SCL cycles */
> > +static inline void machxo2_delay(const struct machxo2_i2c *i2c,
> > + unsigned int cycles)
> > +{
> > + udelay(machxo2_cycles_to_usecs(i2c, cycles));
>
> can we use fsleep here?
Outside of machxo2_recover, this is only run from the hrtimer handler, which is
softirq context, right? In any case, the MachXO2 I2C controller has some
unfortunate timing-related fragility, so waiting longer than expected could be
bad.
Switching to fsleep just for machxo2_recover would be an option, which is also
the only place where a machxo2_delay() with cycles > 2 happens.
>
> > +}
>
> ...
>
> > +static enum machxo2_i2c_state
> > +machxo2_write(struct machxo2_i2c *i2c, u8 stat)
> > +{
> > + struct i2c_msg *msg = i2c->msg;
> > +
> > + if (stat & I2C_SR_TRRDY) {
>
> if you do
>
> if (!(stat & I2C_SR_TRRDY))
> return STATE_WRITE;
>
> you can save a level of indentation.
>
> > + bool eom = (i2c->pos == msg->len);
> > +
> > + if (eom) {
> > + machxo2_delay(i2c, 1);
> > + stat = machxo2_get(i2c, I2C_SR);
> > + }
> > +
> > + if ((i2c->pos > 0 || eom) && (stat & I2C_SR_RARC)) {
> > + dev_dbg(&i2c->adap.dev, "No ACK at %d\n", i2c->pos);
> > + return machxo2_error(i2c, -ENXIO);
> > + }
> > +
> > + if (eom)
> > + return machxo2_end_of_message(i2c);
> > +
> > + machxo2_set(i2c, I2C_TXDR, msg->buf[i2c->pos++]);
> > + machxo2_set(i2c, I2C_CMDR, I2C_CMDR_WR);
> > +
> > + machxo2_reset_timer_wait(i2c);
> > + }
> > +
> > + return STATE_WRITE;
> > +}
>
> ...
>
> > +static irqreturn_t machxo2_isr(int irq, void *dev_id)
> > +{
> > + struct machxo2_i2c *i2c = dev_id;
> > + u8 irq_stat = machxo2_get(i2c, I2C_IRQ);
> > +
> > + if (!(irq_stat & I2C_IRQ_MASK))
> > + return IRQ_NONE;
> > +
> > + /*
> > + * Due to a race condition in the I2C controller, no edge on the IRQ
> > + * line may be generated if an event comes in right at the moment when
> > + * the IRQs are cleared. Loop to ensure that IRQs are actually cleared.
> > + */
> > + do {
> > + machxo2_set(i2c, I2C_IRQ, I2C_IRQ_MASK);
> > + irq_stat = machxo2_get(i2c, I2C_IRQ);
> > + } while (irq_stat & I2C_IRQ_MASK);
> > +
> > + spin_lock(&i2c->lock);
> > + hrtimer_start(&i2c->timer, 0, HRTIMER_MODE_REL);
> > + spin_unlock(&i2c->lock);
>
> why do we need spin_locks here?
This prevents a race against the following code in machxo2_restart_timer():
if (!hrtimer_is_queued(&i2c->timer))
hrtimer_forward_now(&i2c->timer, i2c->timer_wait);
We do not want to hrtimer_forward_now() if the timer is already queued, so we
never lengthen the time until the next expiry.
>
> > + return IRQ_HANDLED;
> > +}
>
> ...
>
> > +static int machxo2_init(struct machxo2_i2c *i2c)
> > +{
> > + unsigned int prescale;
> > +
> > + /* Make sure the device is disabled */
> > + machxo2_set(i2c, I2C_CR, 0);
> > +
> > + /* I2C bus frequencies officially supported by MachXO2 are 400, 100 and 50 kHz */
> > + if (i2c->bus_khz >= 400)
> > + i2c->bus_khz = 400;
> > + else if (i2c->bus_khz >= 100)
> > + i2c->bus_khz = 100;
> > + else
> > + i2c->bus_khz = 50;
> > +
> > + prescale = DIV_ROUND_UP(i2c->clock_khz, 4 * i2c->bus_khz);
> > + if (prescale > 0x3ff) {
>
> can you please explain this check?
The register field is 10 bits wide, making 0x3ff its maximum value. Should I add
a comment?
>
> > + dev_err(&i2c->adap.dev, "unsupported prescale: %d\n",
> > + prescale);
> > + return -EINVAL;
> > + }
> > +
> > + machxo2_set(i2c, I2C_BR0, prescale & 0xff);
> > + machxo2_set(i2c, I2C_BR1, prescale >> 8);
> > +
> > + machxo2_set(i2c, I2C_IRQEN, 0);
> > + machxo2_set(i2c, I2C_CR, I2C_CR_I2CEN);
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int machxo2_i2c_probe(struct platform_device *pdev)
> > +{
> > + struct machxo2_i2c_platform_data *pdata;
> > + struct machxo2_i2c *i2c;
> > + struct resource *res;
> > + void __iomem *regs;
> > + int ret;
> > +
> > + i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> > + if (!i2c)
> > + return -ENOMEM;
> > +
> > + pdata = dev_get_platdata(&pdev->dev);
> > + if (!pdata || !pdata->clock_khz)
> > + return -EINVAL;
> > +
> > + i2c->clock_khz = pdata->clock_khz;
> > + i2c->bus_khz = pdata->bus_khz;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> > + if (IS_ERR(res))
> > + return PTR_ERR(res);
> > +
> > + if (!devm_request_region(&pdev->dev, res->start, resource_size(res),
> > + pdev->name)) {
> > + dev_err(&pdev->dev, "Can't get I/O resource.\n");
> > + return -EBUSY;
>
> please, use dev_err_probe() here.
>
> > + }
> > +
> > + regs = devm_ioport_map(&pdev->dev, res->start, resource_size(res));
> > + if (!regs) {
> > + dev_err(&pdev->dev, "Can't map I/O resource.\n");
> > + return -EBUSY;
> > + }
> > +
> > + i2c->regmap = devm_regmap_init_mmio(&pdev->dev, regs, &machxo2_regmap_config);
> > + if (IS_ERR(i2c->regmap))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(i2c->regmap),
> > + "Unable to initialize register map\n");
> > +
> > + i2c->irq = platform_get_irq_optional(pdev, 0);
> > + if (i2c->irq < 0 && i2c->irq != -ENXIO)
> > + return i2c->irq;
> > +
> > + ret = machxo2_init(i2c);
> > + if (ret)
> > + return ret;
> > +
> > + init_waitqueue_head(&i2c->wait);
> > + spin_lock_init(&i2c->lock);
> > + hrtimer_setup(&i2c->timer, machxo2_handle, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +
> > + if (i2c->irq > 0) {
> > + ret = devm_request_irq(&pdev->dev, i2c->irq, machxo2_isr, 0,
> > + pdev->name, i2c);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Cannot claim IRQ\n");
> > + machxo2_set(i2c, I2C_CR, 0);
> > + return ret;
>
> Do we need to write '0' to I2C_CR here?
machxo2_init() would have set I2C_CR_I2CEN here already; this resets it to 0. I
will check if moving the machxo2_set(i2c, I2C_CR, I2C_CR_I2CEN) to a later point
in the probe is possible.
Best,
Matthias
>
> Thanks,
> Andi
>
> > + }
> > +
> > + machxo2_set(i2c, I2C_IRQEN, I2C_IRQ_MASK);
> > +
> > + dev_info(&pdev->dev,
> > + "Using IRQ %d, bus speed %dKHz, clock %dKHz.\n",
> > + i2c->irq, i2c->bus_khz, i2c->clock_khz);
> > + } else {
> > + dev_info(&pdev->dev,
> > + "Running in polling mode, bus speed %dKHz, clock %dKHz.\n",
> > + i2c->bus_khz, i2c->clock_khz);
> > + }
> > +
> > + platform_set_drvdata(pdev, i2c);
> > +
> > + strscpy(i2c->adap.name, "i2c-machxo2", sizeof(i2c->adap.name));
> > + i2c->adap.algo = &machxo2_algorithm;
> > + i2c->adap.dev.parent = &pdev->dev;
> > + i2c->adap.nr = pdev->id;
> > + i2c->adap.owner = THIS_MODULE;
> > + i2c_set_adapdata(&i2c->adap, i2c);
> > +
> > + ret = i2c_add_adapter(&i2c->adap);
> > + if (ret) {
> > + machxo2_set(i2c, I2C_CR, 0);
> > + machxo2_set(i2c, I2C_IRQEN, 0);
> > +
> > + if (i2c->irq > 0)
> > + disable_irq(i2c->irq);
> > + hrtimer_cancel(&i2c->timer);
> > +
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
Powered by blists - more mailing lists