0

I have a kernel module which provides a gpiochip, accessible from userspace using libgpiod (gpioget/set/mon, etc.).

After moving from kernel 5.10 to 6.1, I get a new warning: not an immutable chip, please consider fixing it!.

The function which generates this message either:

  • returns early if you don't set IRQCHIP_IMMUTABLE (which I don't want to, as this is GPIO for userspace), or
  • warns and continues.

How am I meant to avoid both the warning and the early return?

static void gpiochip_set_irq_hooks(struct gpio_chip *gc)
{
    struct irq_chip *irqchip = gc->irq.chip;

    if (irqchip->flags & IRQCHIP_IMMUTABLE)
        return;

    chip_warn(gc, "not an immutable chip, please consider fixing it!\n");

    if (!irqchip->irq_request_resources &&
        !irqchip->irq_release_resources) {
        irqchip->irq_request_resources = gpiochip_irq_reqres;
        irqchip->irq_release_resources = gpiochip_irq_relres;
    }
    if (WARN_ON(gc->irq.irq_enable))
        return;
    /* Check if the irqchip already has this hook... */
    if (irqchip->irq_enable == gpiochip_irq_enable ||
        irqchip->irq_mask == gpiochip_irq_mask) {
        /*
         * ...and if so, give a gentle warning that this is bad
         * practice.
         */
        chip_info(gc,
              "detected irqchip that is shared with multiple gpiochips: please fix the driver.\n");
        return;
    }

    if (irqchip->irq_disable) {
        gc->irq.irq_disable = irqchip->irq_disable;
        irqchip->irq_disable = gpiochip_irq_disable;
    } else {
        gc->irq.irq_mask = irqchip->irq_mask;
        irqchip->irq_mask = gpiochip_irq_mask;
    }

    if (irqchip->irq_enable) {
        gc->irq.irq_enable = irqchip->irq_enable;
        irqchip->irq_enable = gpiochip_irq_enable;
    } else {
        gc->irq.irq_unmask = irqchip->irq_unmask;
        irqchip->irq_unmask = gpiochip_irq_unmask;
    }
}

Response to 0andriy.

The reason that I do not use IRQCHIP_IMMUTABLE is because I have read that you must not use IRQCHIP_IMMUTABLE if the gpiochip/irqchip which you are providing needs to be accessible from userspace (through libgpiod) and that you should only use IRQCHIP_IMMUTABLE if your chip is used solely by other kernel modules.

I could well be mistaken here, so please link to any sources which say I should specify IRQCHIP_IMMUTABLE for a gpiochip which is to be queried by userspace processes.

The line where I could, but do not use the flag IRQCHIP_IMMUTABLE is:

        ret = devm_request_threaded_irq(&client->dev, client->irq,
                    NULL, my_pmu_irq, IRQF_ONESHOT |
                    IRQF_TRIGGER_FALLING | IRQF_SHARED,
                    dev_name(&client->dev), pmu);

Please find below the initialisation code for the GPIO function of my driver. It is a PMU chip which has GPIO capabilities, accessed from the kernel via an I2C bus. There is an IRQ line, which the PMU uses to signal to the kernel that something has changed. The my_pmu driver then queries the PMU via I2C to get the new GPIO values.

The Device tree is:

    my_pmu: my_pmu@43 {
        compatible = "my,my-pmu";
        reg = <0x43>;
        status = "okay";
        gpio-controller;

        interrupt-parent = <&gpio5>;
        interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
    };

The GPIO/IRQ part of the driver initialisation code is:


    //
    // Set-up as GPIO device
    //

    /* Allocate, initialize, and register this gpio_chip. */
    pmu = devm_kzalloc(&client->dev, sizeof(struct my_pmu), GFP_KERNEL);
    if (!pmu)
        return -ENOMEM;

    printk("about to init mutex\n"); 
    mutex_init(&pmu->lock);

    pmu->chip.base          = -1;
    pmu->chip.can_sleep     = true;
    pmu->chip.parent        = &client->dev;
    pmu->chip.owner         = THIS_MODULE;
    pmu->chip.get           = my_pmu_get;
    pmu->chip.set           = my_pmu_set;
    /*
     * direction_*() function uneeded as pin functions cannot be changed
     *
     * If direction_output() exists, it will be called *instead* of set(),
     * leading to profound confusion.
     */
    //pmu->chip.direction_input = my_pmu_direction_input;
    //pmu->chip.direction_output    = my_pmu_direction_output;
    pmu->chip.get_direction     = my_pmu_get_direction;
    pmu->chip.ngpio         = id->driver_data;

    pmu->chip.label = client->name;

    pmu->client = client;
    i2c_set_clientdata(client, pmu);

    /* first read of input and output registers */
    // TODO: Refactor this so that interrupts and initialisation use the same code.
    new_numbered_inputs = i2c_read_word(client, PMU_REG_NUMBERED_INPUTS);
    printk("new_numbered_inputs: %08X\n", new_numbered_inputs);
    // update only bits 0-7
    printk("mask: %08X\n", PMU_MASK_NUMBERED_INPUTS);
    pmu->status = (pmu->status & ~PMU_MASK_NUMBERED_INPUTS) | new_numbered_inputs;
    printk("pmu->status: %08X\n", pmu->status);

    new_named_inputs = i2c_read_word(client, PMU_REG_NAMED_INPUTS);
    printk("new_named_inputs: %08X\n", new_named_inputs);
    // update only bits 8-17, remembering the shift
    printk("mask: %08X\n", PMU_MASK_NAMED_INPUTS);
    pmu->status = (pmu->status & ~PMU_MASK_NAMED_INPUTS) | (new_named_inputs << PMU_OFFSET_NAMED_INPUTS);
    printk("pmu->status: %08X\n", pmu->status);

    new_pseudo_inputs = i2c_read_word(client, PMU_REG_PSEUDO_INPUTS);
    printk("new_pseudo_inputs: %08X\n", new_pseudo_inputs);
    // update only bits 18-19, remembering the shift
    printk("mask: %08X\n", PMU_MASK_PSEUDO_INPUTS);
    pmu->status = (pmu->status & ~PMU_MASK_PSEUDO_INPUTS) | (new_pseudo_inputs << PMU_OFFSET_PSEUDO_INPUTS);
    printk("pmu->status: %08X\n", pmu->status);

    new_outputs = i2c_read_word(client, PMU_REG_OUTPUTS);
    printk("new_outputs: %08X\n", new_outputs);
    // update only bits 20-23, remembering the shift
    printk("mask: %08X\n", PMU_MASK_OUTPUTS);
    pmu->status = (pmu->status & ~PMU_MASK_OUTPUTS) | (new_named_inputs << PMU_OFFSET_OUTPUTS);
    printk("pmu->status: %08X\n", pmu->status);

    /* Enable irqchip if we have an interrupt */
    if (client->irq) {
        struct gpio_irq_chip *girq;

        pmu->irqchip.name = "my_pmu";
        pmu->irqchip.irq_enable = my_pmu_irq_enable;
        pmu->irqchip.irq_disable = my_pmu_irq_disable;
        pmu->irqchip.irq_ack = noop;
        pmu->irqchip.irq_mask = noop;
        pmu->irqchip.irq_unmask = noop;
        pmu->irqchip.irq_set_wake = my_pmu_irq_set_wake;
        pmu->irqchip.irq_bus_lock = my_pmu_irq_bus_lock;
        pmu->irqchip.irq_bus_sync_unlock = my_pmu_irq_bus_sync_unlock;

        printk("%s:%d %s before devm_request_threaded_irq() \n", __FILE__, __LINE__, __func__);
        ret = devm_request_threaded_irq(&client->dev, client->irq,
                    NULL, my_pmu_irq, IRQF_ONESHOT |
                    IRQF_TRIGGER_FALLING | IRQF_SHARED,
                    dev_name(&client->dev), pmu);
        printk("%s:%d %s after devm_request_threaded_irq(), ret = %i \n", __FILE__, __LINE__, __func__, ret);
        if (ret) {
            printk("devm_request_threaded_irq failed\n"); 
            goto fail;
        }

        girq = &pmu->chip.irq;
        girq->chip = &pmu->irqchip;
        /* This will let us handle the parent IRQ in the driver */
        girq->parent_handler = NULL;
        girq->num_parents = 0;
        girq->parents = NULL;
        girq->default_type = IRQ_TYPE_NONE;
        girq->handler = handle_level_irq;
        girq->threaded = true;
    } else {
        printk("%s:%d %s client->irq == 0\n", __FILE__, __LINE__, __func__);
    }

    printk("%s:%d %s before devm_gpiochip_add_data() \n", __FILE__, __LINE__, __func__);
    ret = devm_gpiochip_add_data(&client->dev, &pmu->chip, pmu);
    printk("%s:%d %s after devm_gpiochip_add_data(), ret = %i \n", __FILE__, __LINE__, __func__, ret);
    if (ret < 0) {
        printk("devm_gpiochip_add_data failed\n");
        goto fail;
    }
6
  • 2
    The main misunderstanding here is this passage in your Q: which I don't want to, as this is GPIO for userspace. How are the two related? And the code you shown is in the kernel, show your code which needs to be fixed. (FWIW, I know what to do and able to answer the Q, but without seeing the exact pieces, starting from the definition of the GPIO IRQ chip structure, it's not so possible) Commented Apr 25 at 21:51
  • @0andriy Thanks for your comment; I've added my response in the question as it's too long for the comments. Commented Apr 28 at 7:48
  • Apply something like this to your code: web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/…. Commented Apr 28 at 19:21
  • Second issue you have with the code is the IRQF_TRIGGER_FALLING (or any other triggering flags in devm_request_threaded_irq(). Currently it overrides whateveer in DT, which is incorrect approach. Then few minor things: 1) usesizeof(*pmu) which is more robust against changes; 2) use pr_info() or alike instead of plain printk(), but actually you have a device pointer, use dev_info() or alike; 3) I don't see a context, but usually the code in ->probe() like ret = devm_...(); if (ret) { ...; goto fail; } is wrong. Commented Apr 28 at 19:21
  • The goto fail part must not be after devm_*() calls as it signals about wrong order when cleaning resources (error parh in ->probe() or full ->remove() callback. Commented Apr 28 at 19:21

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.