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;
}
IRQF_TRIGGER_FALLING(or any other triggering flags indevm_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) usepr_info()or alike instead of plainprintk(), but actually you have a device pointer, usedev_info()or alike; 3) I don't see a context, but usually the code in->probe()likeret = devm_...(); if (ret) { ...; goto fail; }is wrong.goto failpart must not be afterdevm_*()calls as it signals about wrong order when cleaning resources (error parh in->probe()or full->remove()callback.