Re: [PATCH v5 4/5] net: wan: fsl_qmc_hdlc: Add runtime timeslots changes support

2024-02-29 Thread Herve Codina
Hi Andy,

On Thu, 29 Feb 2024 17:20:59 +0200
Andy Shevchenko  wrote:

> On Thu, Feb 29, 2024 at 03:15:52PM +0100, Herve Codina wrote:
> > QMC channels support runtime timeslots changes but nothing is done at
> > the QMC HDLC driver to handle these changes.
> > 
> > Use existing IFACE ioctl in order to configure the timeslots to use.  
> 
> ...
> 
> > +   bitmap_scatter(ts_mask, map, ts_mask_avail, 64);  
> 
> Wondering if we may have returned value more useful and hence having 
> something like
> 
>   n = bitmap_scatter(...);

I thought about it.

In bitmap_{scatter,gather}(dst, src, mask, nbits), only returning the
weight of the third parameter (i.e. mask) can be efficient regarding to the
for_each_set_bit() loop done in the functions.
For dst parameter, we need to add a counter in the loop to count the number
of bit set depending on the test_bit() result. Will this be more efficient
than a call to bitmap_weight() ?

Also, in my case, the third parameter is ts_mask_avail and I don't need
its weight.

I thing users that need to have the dst or src weight should call
bitmap_weight() themselves as this is users context dependent.

bitmap_{scatter,gather}(dst, src, mask, nbits) can be improved later with
no impact to current users (except performance).

That's why I concluded to return nothing from bitmap_{scatter,gather} when
I took the old existing patches.

> 
> > +   if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {  
> 
>   if (n != ...) {
> 
> ?
> 
> > +   dev_err(qmc_hdlc->dev, "Cannot translate timeslots %64pb -> 
> > (%64pb, %64pb)\n",
> > +   map, ts_mask_avail, ts_mask);
> > +   return -EINVAL;
> > +   }  
> 
> ...
> 
> > +   bitmap_gather(map, ts_mask, ts_mask_avail, 64);
> > +
> > +   if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
> > +   dev_err(qmc_hdlc->dev, "Cannot translate timeslots (%64pb, 
> > %64pb) -> %64pb\n",
> > +   ts_mask_avail, ts_mask, map);
> > +   return -EINVAL;
> > +   }  
> 
> Ditto.
> 

Best regards,
Hervé


Re: [PATCH v5 4/5] net: wan: fsl_qmc_hdlc: Add runtime timeslots changes support

2024-02-29 Thread Andy Shevchenko
On Thu, Feb 29, 2024 at 03:15:52PM +0100, Herve Codina wrote:
> QMC channels support runtime timeslots changes but nothing is done at
> the QMC HDLC driver to handle these changes.
> 
> Use existing IFACE ioctl in order to configure the timeslots to use.

...

> + bitmap_scatter(ts_mask, map, ts_mask_avail, 64);

Wondering if we may have returned value more useful and hence having something 
like

n = bitmap_scatter(...);

> + if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {

if (n != ...) {

?

> + dev_err(qmc_hdlc->dev, "Cannot translate timeslots %64pb -> 
> (%64pb, %64pb)\n",
> + map, ts_mask_avail, ts_mask);
> + return -EINVAL;
> + }

...

> + bitmap_gather(map, ts_mask, ts_mask_avail, 64);
> +
> + if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
> + dev_err(qmc_hdlc->dev, "Cannot translate timeslots (%64pb, 
> %64pb) -> %64pb\n",
> + ts_mask_avail, ts_mask, map);
> + return -EINVAL;
> + }

Ditto.

-- 
With Best Regards,
Andy Shevchenko




[PATCH v5 4/5] net: wan: fsl_qmc_hdlc: Add runtime timeslots changes support

2024-02-29 Thread Herve Codina
QMC channels support runtime timeslots changes but nothing is done at
the QMC HDLC driver to handle these changes.

Use existing IFACE ioctl in order to configure the timeslots to use.

Signed-off-by: Herve Codina 
Reviewed-by: Christophe Leroy 
Acked-by: Jakub Kicinski 
---
 drivers/net/wan/fsl_qmc_hdlc.c | 151 -
 1 file changed, 150 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c
index b9e067101171..80998ad89088 100644
--- a/drivers/net/wan/fsl_qmc_hdlc.c
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -8,6 +8,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -33,6 +34,7 @@ struct qmc_hdlc {
struct qmc_hdlc_desc tx_descs[8];
unsigned int tx_out;
struct qmc_hdlc_desc rx_descs[4];
+   u32 slot_map;
 };
 
 static struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
@@ -197,6 +199,144 @@ static netdev_tx_t qmc_hdlc_xmit(struct sk_buff *skb, 
struct net_device *netdev)
return NETDEV_TX_OK;
 }
 
+static int qmc_hdlc_xlate_slot_map(struct qmc_hdlc *qmc_hdlc,
+  u32 slot_map, struct qmc_chan_ts_info 
*ts_info)
+{
+   DECLARE_BITMAP(ts_mask_avail, 64);
+   DECLARE_BITMAP(ts_mask, 64);
+   DECLARE_BITMAP(map, 64);
+
+   /* Tx and Rx available masks must be identical */
+   if (ts_info->rx_ts_mask_avail != ts_info->tx_ts_mask_avail) {
+   dev_err(qmc_hdlc->dev, "tx and rx available timeslots mismatch 
(0x%llx, 0x%llx)\n",
+   ts_info->rx_ts_mask_avail, ts_info->tx_ts_mask_avail);
+   return -EINVAL;
+   }
+
+   bitmap_from_u64(ts_mask_avail, ts_info->rx_ts_mask_avail);
+   bitmap_from_u64(map, slot_map);
+   bitmap_scatter(ts_mask, map, ts_mask_avail, 64);
+
+   if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
+   dev_err(qmc_hdlc->dev, "Cannot translate timeslots %64pb -> 
(%64pb, %64pb)\n",
+   map, ts_mask_avail, ts_mask);
+   return -EINVAL;
+   }
+
+   bitmap_to_arr64(_info->tx_ts_mask, ts_mask, 64);
+   ts_info->rx_ts_mask = ts_info->tx_ts_mask;
+   return 0;
+}
+
+static int qmc_hdlc_xlate_ts_info(struct qmc_hdlc *qmc_hdlc,
+ const struct qmc_chan_ts_info *ts_info, u32 
*slot_map)
+{
+   DECLARE_BITMAP(ts_mask_avail, 64);
+   DECLARE_BITMAP(ts_mask, 64);
+   DECLARE_BITMAP(map, 64);
+   u32 array32[2];
+
+   /* Tx and Rx masks and available masks must be identical */
+   if (ts_info->rx_ts_mask_avail != ts_info->tx_ts_mask_avail) {
+   dev_err(qmc_hdlc->dev, "tx and rx available timeslots mismatch 
(0x%llx, 0x%llx)\n",
+   ts_info->rx_ts_mask_avail, ts_info->tx_ts_mask_avail);
+   return -EINVAL;
+   }
+   if (ts_info->rx_ts_mask != ts_info->tx_ts_mask) {
+   dev_err(qmc_hdlc->dev, "tx and rx timeslots mismatch (0x%llx, 
0x%llx)\n",
+   ts_info->rx_ts_mask, ts_info->tx_ts_mask);
+   return -EINVAL;
+   }
+
+   bitmap_from_u64(ts_mask_avail, ts_info->rx_ts_mask_avail);
+   bitmap_from_u64(ts_mask, ts_info->rx_ts_mask);
+   bitmap_gather(map, ts_mask, ts_mask_avail, 64);
+
+   if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
+   dev_err(qmc_hdlc->dev, "Cannot translate timeslots (%64pb, 
%64pb) -> %64pb\n",
+   ts_mask_avail, ts_mask, map);
+   return -EINVAL;
+   }
+
+   bitmap_to_arr32(array32, map, 64);
+   if (array32[1]) {
+   dev_err(qmc_hdlc->dev, "Slot map out of 32bit (%64pb, %64pb) -> 
%64pb\n",
+   ts_mask_avail, ts_mask, map);
+   return -EINVAL;
+   }
+
+   *slot_map = array32[0];
+   return 0;
+}
+
+static int qmc_hdlc_set_iface(struct qmc_hdlc *qmc_hdlc, int if_iface, const 
te1_settings *te1)
+{
+   struct qmc_chan_ts_info ts_info;
+   int ret;
+
+   ret = qmc_chan_get_ts_info(qmc_hdlc->qmc_chan, _info);
+   if (ret) {
+   dev_err(qmc_hdlc->dev, "get QMC channel ts info failed %d\n", 
ret);
+   return ret;
+   }
+   ret = qmc_hdlc_xlate_slot_map(qmc_hdlc, te1->slot_map, _info);
+   if (ret)
+   return ret;
+
+   ret = qmc_chan_set_ts_info(qmc_hdlc->qmc_chan, _info);
+   if (ret) {
+   dev_err(qmc_hdlc->dev, "set QMC channel ts info failed %d\n", 
ret);
+   return ret;
+   }
+
+   qmc_hdlc->slot_map = te1->slot_map;
+
+   return 0;
+}
+
+static int qmc_hdlc_ioctl(struct net_device *netdev, struct if_settings *ifs)
+{
+   struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+   te1_settings te1;
+
+   switch (ifs->type) {
+   case IF_GET_IFACE:
+   ifs->type = IF_IFACE_E1;
+   if (ifs->size < sizeof(te1)) {
+