| Message ID | CAFXsbZp5A7FHoXPA6Rg8XqZPD9NXmSeZZb-RsEGXnktbo04GOw@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: sfp: Unique GPIO interrupt names | expand |
On Mon, Jul 06, 2020 at 12:38:37PM -0700, Chris Healy wrote: > Dynamically generate a unique GPIO interrupt name, based on the > device name and the GPIO name. For example: > > 103: 0 sx1503q 12 Edge sff2-los > 104: 0 sx1503q 13 Edge sff3-los > > The sffX indicates the SFP the loss of signal GPIO is associated with. Hi Chris For netdev, please put inside the [PATCH] part of the subject, which tree this is for, i.e. net-next. > Signed-off-by: Chris Healy <cphealy@gmail.com> > --- > drivers/net/phy/sfp.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > index 73c2969f11a4..9b03c7229320 100644 > --- a/drivers/net/phy/sfp.c > +++ b/drivers/net/phy/sfp.c > @@ -220,6 +220,7 @@ struct sfp { > struct phy_device *mod_phy; > const struct sff_data *type; > u32 max_power_mW; > + char sfp_irq_name[32]; > > unsigned int (*get_state)(struct sfp *); > void (*set_state)(struct sfp *, unsigned int); > @@ -2349,12 +2350,15 @@ static int sfp_probe(struct platform_device *pdev) > continue; > } > > + snprintf(sfp->sfp_irq_name, sizeof(sfp->sfp_irq_name), > + "%s-%s", dev_name(sfp->dev), gpio_of_names[i]); > + This is perfectly O.K, but you could consider using devm_kasprintf(). That will allocate as much memory as needed for the string, and hence avoid truncation issues, which we have seen before with other interrupt names. Andrew
On Mon, Jul 06, 2020 at 12:38:37PM -0700, Chris Healy wrote: > Dynamically generate a unique GPIO interrupt name, based on the > device name and the GPIO name. For example: > > 103: 0 sx1503q 12 Edge sff2-los > 104: 0 sx1503q 13 Edge sff3-los > > The sffX indicates the SFP the loss of signal GPIO is associated with. > > Signed-off-by: Chris Healy <cphealy@gmail.com> This doesn't work in all cases. > --- > drivers/net/phy/sfp.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > index 73c2969f11a4..9b03c7229320 100644 > --- a/drivers/net/phy/sfp.c > +++ b/drivers/net/phy/sfp.c > @@ -220,6 +220,7 @@ struct sfp { > struct phy_device *mod_phy; > const struct sff_data *type; > u32 max_power_mW; > + char sfp_irq_name[32]; > > unsigned int (*get_state)(struct sfp *); > void (*set_state)(struct sfp *, unsigned int); > @@ -2349,12 +2350,15 @@ static int sfp_probe(struct platform_device *pdev) > continue; > } > > + snprintf(sfp->sfp_irq_name, sizeof(sfp->sfp_irq_name), > + "%s-%s", dev_name(sfp->dev), gpio_of_names[i]); sfp_irq_name will be overwritten for each GPIO IRQ claimed, which means all IRQs for a particular cage will end up with the same name. sfp_irq_name[] therefore needs to be an array of names, one per input. Thanks.
On Mon, Jul 6, 2020 at 1:07 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Mon, Jul 06, 2020 at 12:38:37PM -0700, Chris Healy wrote: > > Dynamically generate a unique GPIO interrupt name, based on the > > device name and the GPIO name. For example: > > > > 103: 0 sx1503q 12 Edge sff2-los > > 104: 0 sx1503q 13 Edge sff3-los > > > > The sffX indicates the SFP the loss of signal GPIO is associated with. > > Hi Chris > > For netdev, please put inside the [PATCH] part of the subject, which > tree this is for, i.e. net-next. Will do. > > > Signed-off-by: Chris Healy <cphealy@gmail.com> > > --- > > drivers/net/phy/sfp.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > > index 73c2969f11a4..9b03c7229320 100644 > > --- a/drivers/net/phy/sfp.c > > +++ b/drivers/net/phy/sfp.c > > @@ -220,6 +220,7 @@ struct sfp { > > struct phy_device *mod_phy; > > const struct sff_data *type; > > u32 max_power_mW; > > + char sfp_irq_name[32]; > > > > unsigned int (*get_state)(struct sfp *); > > void (*set_state)(struct sfp *, unsigned int); > > @@ -2349,12 +2350,15 @@ static int sfp_probe(struct platform_device *pdev) > > continue; > > } > > > > + snprintf(sfp->sfp_irq_name, sizeof(sfp->sfp_irq_name), > > + "%s-%s", dev_name(sfp->dev), gpio_of_names[i]); > > + > > This is perfectly O.K, but you could consider using > devm_kasprintf(). That will allocate as much memory as needed for the > string, and hence avoid truncation issues, which we have seen before > with other interrupt names. I'll give this a try for the next version of this patch. > > Andrew
On Mon, Jul 6, 2020 at 1:42 PM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Mon, Jul 06, 2020 at 12:38:37PM -0700, Chris Healy wrote: > > Dynamically generate a unique GPIO interrupt name, based on the > > device name and the GPIO name. For example: > > > > 103: 0 sx1503q 12 Edge sff2-los > > 104: 0 sx1503q 13 Edge sff3-los > > > > The sffX indicates the SFP the loss of signal GPIO is associated with. > > > > Signed-off-by: Chris Healy <cphealy@gmail.com> > > This doesn't work in all cases. > > > --- > > drivers/net/phy/sfp.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > > index 73c2969f11a4..9b03c7229320 100644 > > --- a/drivers/net/phy/sfp.c > > +++ b/drivers/net/phy/sfp.c > > @@ -220,6 +220,7 @@ struct sfp { > > struct phy_device *mod_phy; > > const struct sff_data *type; > > u32 max_power_mW; > > + char sfp_irq_name[32]; > > > > unsigned int (*get_state)(struct sfp *); > > void (*set_state)(struct sfp *, unsigned int); > > @@ -2349,12 +2350,15 @@ static int sfp_probe(struct platform_device *pdev) > > continue; > > } > > > > + snprintf(sfp->sfp_irq_name, sizeof(sfp->sfp_irq_name), > > + "%s-%s", dev_name(sfp->dev), gpio_of_names[i]); > > sfp_irq_name will be overwritten for each GPIO IRQ claimed, which means > all IRQs for a particular cage will end up with the same name. > sfp_irq_name[] therefore needs to be an array of names, one per input. Good point. I'll add the necessary support to deal with this case and test on my side with a hacked up devicetree file providing some additional GPIOs and include this in the next version of the patch. > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index 73c2969f11a4..9b03c7229320 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -220,6 +220,7 @@ struct sfp { struct phy_device *mod_phy; const struct sff_data *type; u32 max_power_mW; + char sfp_irq_name[32]; unsigned int (*get_state)(struct sfp *); void (*set_state)(struct sfp *, unsigned int); @@ -2349,12 +2350,15 @@ static int sfp_probe(struct platform_device *pdev) continue; } + snprintf(sfp->sfp_irq_name, sizeof(sfp->sfp_irq_name), + "%s-%s", dev_name(sfp->dev), gpio_of_names[i]); + err = devm_request_threaded_irq(sfp->dev, sfp->gpio_irq[i], NULL, sfp_irq, IRQF_ONESHOT | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, - dev_name(sfp->dev), sfp); + sfp->sfp_irq_name, sfp); if (err) { sfp->gpio_irq[i] = 0; sfp->need_poll = true;
Dynamically generate a unique GPIO interrupt name, based on the device name and the GPIO name. For example: 103: 0 sx1503q 12 Edge sff2-los 104: 0 sx1503q 13 Edge sff3-los The sffX indicates the SFP the loss of signal GPIO is associated with. Signed-off-by: Chris Healy <cphealy@gmail.com> --- drivers/net/phy/sfp.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)