| Message ID | 4af4f6c5-d7da-4735-9ef5-ee3c34f7eae6@cyberdanube.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: sim: added configfs option for static base | expand |
On Fri, Feb 28, 2025 at 10:02 PM Sebastian Dietz <s.dietz@cyberdanube.com> wrote: > > On 28.02.25 16:43, Bartosz Golaszewski wrote: > > On Fri, Feb 28, 2025 at 2:54 PM Sebastian Dietz <s.dietz@cyberdanube.com> wrote: > >> > >> On 28.02.25 14:22, Bartosz Golaszewski wrote: > >>> On Fri, Feb 28, 2025 at 1:46 PM Sebastian Dietz <s.dietz@cyberdanube.com> wrote: > >>>> > >>>> To replicate gpio mappings of systems it is sometimes needed to have > >>>> the base at static values. > >>>> > >>> > >>> Can you give me more info on why you'd need that? Static base is > >>> largely a legacy and deprecated feature, there's ongoing effort to > >>> remove it from the kernel. > >>> > >>>> base is treated as unsigned as there doesn't happen to be a > >>>> fwnode_property_read_s32(). > >>>> > >>> > >>> Ha! That's interesting, I wonder why that is. We do have signed > >>> variants for OF-specific properties. > >>> > >>> Bart > >> > >> We are building digital twins for embedded devices for security research. The > >> firmware of these devices often export static gpio pins which we simulate > >> using gpio-sim. With this patch we are able to satisfy these conditions. > >> > >> While the feature may be deprecated, i would argue that it makes sense and > >> fits the nature of a simulator to be able to configure it manually. > >> > >> BR, > >> Sebastian > > > > What kind of digital twins? Using qemu? In any case - I really dislike > > the idea of extending the configfs interface of gpio-sim with an > > attribute to support an option that we're actively trying to remove > > from GPIO core. Unless you can give me a really convincing argument, I > > will allow myself to use my maintainers' right to NAK this one. > > > > Bart > > Exactly, we are analysing the firmware and re-host it in a qemu instance with a > newer kernel. > > First of all thanks for giving me the oppertunity to pitch the option. > > Gpio-sim provides a way to create chips for testing purposes. Some embedded > device developers still rely on sysfs. While the ABI is certainly obsolete and > not actively developed, it is still actively maintained - Which means that in > real-world testing scenarios, it remains relevant. Our experience has shown > that these firmware images are often build with hardcoded sysfs paths in mind > and do not use the character device interface. > My experience is that even when using drivers without a hardcoded GPIO base, people tend to rely on specific sysfs paths just because they tend to remain the same unless some dynamic GPIO expanders come into play. This is of course dangerous. > This feature wouldn't encourage new use of static bases - it would just make > testing existing setups less of a headache. > This is not what I mean. We want to remove the "base" attribute from struct gpio_chip. If we expose it in gpio-sim in its supposedly stable configfs interface, then we'll implicitly agree to keep it around forever. I don't want to do it. > Given that gpio-sim is meant for testing, shouldn't it provide the flexibility > to test in this scenarios? > gpio-sim is for testing the GPIO uAPI interface and any user-space tools. Maybe you should just simulate whatever device you need directly in qemu? Bartosz
diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c index b6c230fab840..9a57e1d46503 100644 --- a/drivers/gpio/gpio-sim.c +++ b/drivers/gpio/gpio-sim.c @@ -38,7 +38,7 @@ #include <linux/types.h> #define GPIO_SIM_NGPIO_MAX 1024 -#define GPIO_SIM_PROP_MAX 4 /* Max 3 properties + sentinel. */ +#define GPIO_SIM_PROP_MAX 5 /* Max 4 properties + sentinel. */ #define GPIO_SIM_NUM_ATTRS 3 /* value, pull and sentinel */ static DEFINE_IDA(gpio_sim_ida); @@ -419,15 +419,21 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev) struct gpio_chip *gc; const char *label; u32 num_lines; + u32 base; int ret; ret = fwnode_property_read_u32(swnode, "ngpios", &num_lines); if (ret) return ret; + if (num_lines > GPIO_SIM_NGPIO_MAX) return -ERANGE; + ret = fwnode_property_read_u32(swnode, "base", &base); + if (ret) + return ret; + ret = fwnode_property_read_string(swnode, "gpio-sim,label", &label); if (ret) { label = devm_kasprintf(dev, GFP_KERNEL, "%s:%pfwP", @@ -474,7 +480,7 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev) return ret; gc = &chip->gc; - gc->base = -1; + gc->base = base; gc->ngpio = num_lines; gc->label = label; gc->owner = THIS_MODULE; @@ -629,6 +635,9 @@ struct gpio_sim_bank { struct list_head siblings; char *label; + + //base is treated as unsigned as there is no read_prop_s32 + unsigned int base; unsigned int num_lines; struct list_head line_list; @@ -885,6 +894,7 @@ gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank, memset(properties, 0, sizeof(properties)); + properties[prop_idx++] = PROPERTY_ENTRY_U32("base", bank->base); properties[prop_idx++] = PROPERTY_ENTRY_U32("ngpios", bank->num_lines); if (gpio_sim_bank_has_label(bank)) @@ -1202,10 +1212,48 @@ gpio_sim_bank_config_num_lines_store(struct config_item *item, CONFIGFS_ATTR(gpio_sim_bank_config_, num_lines); +static ssize_t +gpio_sim_bank_config_base_show(struct config_item *item, char *page) +{ + struct gpio_sim_bank *bank = to_gpio_sim_bank(item); + struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank); + + guard(mutex)(&dev->lock); + + return sprintf(page, "%i\n", bank->base); +} + +static ssize_t +gpio_sim_bank_config_base_store(struct config_item *item, + const char *page, size_t count) +{ + struct gpio_sim_bank *bank = to_gpio_sim_bank(item); + struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank); + unsigned int base; + int ret; + + ret = kstrtoint(page, 0, &base); + if (ret) + return ret; + + + guard(mutex)(&dev->lock); + + if (gpio_sim_device_is_live(dev)) + return -EBUSY; + + bank->base = base; + + return count; +} + +CONFIGFS_ATTR(gpio_sim_bank_config_, base); + static struct configfs_attribute *gpio_sim_bank_config_attrs[] = { &gpio_sim_bank_config_attr_chip_name, &gpio_sim_bank_config_attr_label, &gpio_sim_bank_config_attr_num_lines, + &gpio_sim_bank_config_attr_base, NULL }; @@ -1505,6 +1553,7 @@ gpio_sim_device_config_make_bank_group(struct config_group *group, config_group_init_type_name(&bank->group, name, &gpio_sim_bank_config_group_type); bank->num_lines = 1; + bank->base = -1; bank->parent = dev; INIT_LIST_HEAD(&bank->line_list); list_add_tail(&bank->siblings, &dev->bank_list);
To replicate gpio mappings of systems it is sometimes needed to have the base at static values. base is treated as unsigned as there doesn't happen to be a fwnode_property_read_s32(). Signed-off-by: Sebastian Dietz <s.dietz@cyberdanube.com> --- drivers/gpio/gpio-sim.c | 53 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-)