Re: [PATCH v4 5/5] net: wan: fsl_qmc_hdlc: Add framer support
On Thu, 29 Feb 2024 15:58:06 +0200 Andy Shevchenko wrote: > On Thu, Feb 29, 2024 at 01:56:05PM +0100, Herve Codina wrote: > > On Thu, 22 Feb 2024 17:49:40 +0200 > > Andy Shevchenko wrote: > > > On Thu, Feb 22, 2024 at 03:22:18PM +0100, Herve Codina wrote: > > ... > > > I've got an issue with guard(spinlock_irqsave). > > No, you got an issue with sparse. > > > I have the following warning (make C=1): > > drivers/net/wan/fsl_qmc_hdlc.c:49:12: warning: context imbalance in > > 'qmc_hdlc_framer_set_carrier' - wrong count at exit > > > > I also tried to call guard(spinlock_irqsave) before 'if (!qmc_hdlc->framer)' > > but it does not change anything. > > > > Did I miss something in the guard(spinlock_irqsave) usage ? > > You may ignore that for now. Not your problem, no problem in the code. > > https://lore.kernel.org/linux-sparse/8d596a06-9f25-4d9f-8282-deb2d03a6b0a@moroto.mountain/ > Perfect, thanks. Hervé
Re: [PATCH v4 5/5] net: wan: fsl_qmc_hdlc: Add framer support
On Thu, Feb 29, 2024 at 01:56:05PM +0100, Herve Codina wrote: > On Thu, 22 Feb 2024 17:49:40 +0200 > Andy Shevchenko wrote: > > On Thu, Feb 22, 2024 at 03:22:18PM +0100, Herve Codina wrote: ... > I've got an issue with guard(spinlock_irqsave). No, you got an issue with sparse. > I have the following warning (make C=1): > drivers/net/wan/fsl_qmc_hdlc.c:49:12: warning: context imbalance in > 'qmc_hdlc_framer_set_carrier' - wrong count at exit > > I also tried to call guard(spinlock_irqsave) before 'if (!qmc_hdlc->framer)' > but it does not change anything. > > Did I miss something in the guard(spinlock_irqsave) usage ? You may ignore that for now. Not your problem, no problem in the code. https://lore.kernel.org/linux-sparse/8d596a06-9f25-4d9f-8282-deb2d03a6b0a@moroto.mountain/ -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 5/5] net: wan: fsl_qmc_hdlc: Add framer support
Hi Andy,
On Thu, 22 Feb 2024 17:49:40 +0200
Andy Shevchenko wrote:
> On Thu, Feb 22, 2024 at 03:22:18PM +0100, Herve Codina wrote:
> > Add framer support in the fsl_qmc_hdlc driver in order to be able to
> > signal carrier changes to the network stack based on the framer status
> > Also use this framer to provide information related to the E1/T1 line
> > interface on IF_GET_IFACE and configure the line interface according to
> > IF_IFACE_{E1,T1} information.
>
> ...
>
> > +static int qmc_hdlc_framer_set_carrier(struct qmc_hdlc *qmc_hdlc)
> > +{
> > + struct framer_status framer_status;
> > + unsigned long flags;
> > + int ret;
> > +
> > + if (!qmc_hdlc->framer)
> > + return 0;
>
> > + spin_lock_irqsave(_hdlc->carrier_lock, flags);
>
> cleanup.h ?
>
> > + ret = framer_get_status(qmc_hdlc->framer, _status);
> > + if (ret) {
> > + dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
> > + goto end;
> > + }
> > + if (framer_status.link_is_on)
> > + netif_carrier_on(qmc_hdlc->netdev);
> > + else
> > + netif_carrier_off(qmc_hdlc->netdev);
> > +
> > +end:
> > + spin_unlock_irqrestore(_hdlc->carrier_lock, flags);
> > + return ret;
> > +}
>
I've got an issue with guard(spinlock_irqsave).
I changed this code to:
--- 8< ---
static int qmc_hdlc_framer_set_carrier(struct qmc_hdlc *qmc_hdlc)
{
struct framer_status framer_status;
int ret;
if (!qmc_hdlc->framer)
return 0;
guard(spinlock_irqsave)(_hdlc->carrier_lock);
ret = framer_get_status(qmc_hdlc->framer, _status);
if (ret) {
dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
return ret;
}
if (framer_status.link_is_on)
netif_carrier_on(qmc_hdlc->netdev);
else
netif_carrier_off(qmc_hdlc->netdev);
return 0;
}
--- 8< ---
I have the following warning (make C=1):
drivers/net/wan/fsl_qmc_hdlc.c:49:12: warning: context imbalance in
'qmc_hdlc_framer_set_carrier' - wrong count at exit
I also tried to call guard(spinlock_irqsave) before 'if (!qmc_hdlc->framer)'
but it does not change anything.
Did I miss something in the guard(spinlock_irqsave) usage ?
Best regards,
Hervé
Re: [PATCH v4 5/5] net: wan: fsl_qmc_hdlc: Add framer support
On Thu, Feb 22, 2024 at 03:22:18PM +0100, Herve Codina wrote:
> Add framer support in the fsl_qmc_hdlc driver in order to be able to
> signal carrier changes to the network stack based on the framer status
> Also use this framer to provide information related to the E1/T1 line
> interface on IF_GET_IFACE and configure the line interface according to
> IF_IFACE_{E1,T1} information.
...
> +static int qmc_hdlc_framer_set_carrier(struct qmc_hdlc *qmc_hdlc)
> +{
> + struct framer_status framer_status;
> + unsigned long flags;
> + int ret;
> +
> + if (!qmc_hdlc->framer)
> + return 0;
> + spin_lock_irqsave(_hdlc->carrier_lock, flags);
cleanup.h ?
> + ret = framer_get_status(qmc_hdlc->framer, _status);
> + if (ret) {
> + dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
> + goto end;
> + }
> + if (framer_status.link_is_on)
> + netif_carrier_on(qmc_hdlc->netdev);
> + else
> + netif_carrier_off(qmc_hdlc->netdev);
> +
> +end:
> + spin_unlock_irqrestore(_hdlc->carrier_lock, flags);
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
[PATCH v4 5/5] net: wan: fsl_qmc_hdlc: Add framer support
Add framer support in the fsl_qmc_hdlc driver in order to be able to
signal carrier changes to the network stack based on the framer status
Also use this framer to provide information related to the E1/T1 line
interface on IF_GET_IFACE and configure the line interface according to
IF_IFACE_{E1,T1} information.
Signed-off-by: Herve Codina
Reviewed-by: Christophe Leroy
---
drivers/net/wan/fsl_qmc_hdlc.c | 239 -
1 file changed, 235 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c
index 1b7f1d5af273..5e80d3ce4e51 100644
--- a/drivers/net/wan/fsl_qmc_hdlc.c
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -9,6 +9,7 @@
#include
#include
+#include
#include
#include
#include
@@ -28,6 +29,9 @@ struct qmc_hdlc {
struct device *dev;
struct qmc_chan *qmc_chan;
struct net_device *netdev;
+ struct framer *framer;
+ spinlock_t carrier_lock; /* Protect carrier detection */
+ struct notifier_block nb;
bool is_crc32;
spinlock_t tx_lock; /* Protect tx descriptors */
struct qmc_hdlc_desc tx_descs[8];
@@ -41,6 +45,195 @@ static struct qmc_hdlc *netdev_to_qmc_hdlc(struct
net_device *netdev)
return dev_to_hdlc(netdev)->priv;
}
+static int qmc_hdlc_framer_set_carrier(struct qmc_hdlc *qmc_hdlc)
+{
+ struct framer_status framer_status;
+ unsigned long flags;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ spin_lock_irqsave(_hdlc->carrier_lock, flags);
+
+ ret = framer_get_status(qmc_hdlc->framer, _status);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
+ goto end;
+ }
+ if (framer_status.link_is_on)
+ netif_carrier_on(qmc_hdlc->netdev);
+ else
+ netif_carrier_off(qmc_hdlc->netdev);
+
+end:
+ spin_unlock_irqrestore(_hdlc->carrier_lock, flags);
+ return ret;
+}
+
+static int qmc_hdlc_framer_notifier(struct notifier_block *nb, unsigned long
action,
+ void *data)
+{
+ struct qmc_hdlc *qmc_hdlc = container_of(nb, struct qmc_hdlc, nb);
+ int ret;
+
+ if (action != FRAMER_EVENT_STATUS)
+ return NOTIFY_DONE;
+
+ ret = qmc_hdlc_framer_set_carrier(qmc_hdlc);
+ return ret ? NOTIFY_DONE : NOTIFY_OK;
+}
+
+static int qmc_hdlc_framer_start(struct qmc_hdlc *qmc_hdlc)
+{
+ struct framer_status framer_status;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ ret = framer_power_on(qmc_hdlc->framer);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer power-on failed (%d)\n", ret);
+ return ret;
+ }
+
+ /* Be sure that get_status is supported */
+ ret = framer_get_status(qmc_hdlc->framer, _status);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
+ goto framer_power_off;
+ }
+
+ qmc_hdlc->nb.notifier_call = qmc_hdlc_framer_notifier;
+ ret = framer_notifier_register(qmc_hdlc->framer, _hdlc->nb);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer notifier register failed
(%d)\n", ret);
+ goto framer_power_off;
+ }
+
+ return 0;
+
+framer_power_off:
+ framer_power_off(qmc_hdlc->framer);
+ return ret;
+}
+
+static void qmc_hdlc_framer_stop(struct qmc_hdlc *qmc_hdlc)
+{
+ if (!qmc_hdlc->framer)
+ return;
+
+ framer_notifier_unregister(qmc_hdlc->framer, _hdlc->nb);
+ framer_power_off(qmc_hdlc->framer);
+}
+
+static int qmc_hdlc_framer_set_iface(struct qmc_hdlc *qmc_hdlc, int if_iface,
+const te1_settings *te1)
+{
+ struct framer_config config;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ ret = framer_get_config(qmc_hdlc->framer, );
+ if (ret)
+ return ret;
+
+ switch (if_iface) {
+ case IF_IFACE_E1:
+ config.iface = FRAMER_IFACE_E1;
+ break;
+ case IF_IFACE_T1:
+ config.iface = FRAMER_IFACE_T1;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ switch (te1->clock_type) {
+ case CLOCK_DEFAULT:
+ /* Keep current value */
+ break;
+ case CLOCK_EXT:
+ config.clock_type = FRAMER_CLOCK_EXT;
+ break;
+ case CLOCK_INT:
+ config.clock_type = FRAMER_CLOCK_INT;
+ break;
+ default:
+ return -EINVAL;
+ }
+ config.line_clock_rate = te1->clock_rate;
+
+ return framer_set_config(qmc_hdlc->framer, );
+}
+
+static int qmc_hdlc_framer_get_iface(struct qmc_hdlc *qmc_hdlc, int *if_iface,
te1_settings *te1)
+{
+ struct framer_config config;
+ int
