[PATCH 0/4] net: fix bugs and error handling in qinheng ch9200 driver and mii interface

[PATCH 0/4] net: fix bugs and error handling in qinheng ch9200 driver and mii interface

[Qasim Ijaz]

This patch series aims to fix various issues throughout the QinHeng CH9200
driver. This driver fails to handle failures throughout, which in one
case has lead to a uninit access bug found via syzbot. Upon reviewing
the driver I fixed a few more issues which I have included in this patch
series.

Parts of this series are the product of discussions and suggestions I had
from others like Andrew Lunn and Simon Horman, you can view those
discussions below:

Link: <https://lore.kernel.org/all/20250218002443.11731-1-qasdev00@gmail.com/>
Link: <https://lore.kernel.org/all/20250311161157.49065-1-qasdev00@gmail.com/>

Qasim Ijaz (4):
  fix uninitialised access in mii_nway_restart()
  remove extraneous return in control_write() to propagate failures
  improve error handling in get_mac_address()
  add error handling in ch9200_bind()

 drivers/net/mii.c        |  2 ++
 drivers/net/usb/ch9200.c | 59 ++++++++++++++++++++++++++--------------
 2 files changed, 41 insertions(+), 20 deletions(-)

-- 
2.39.5

[PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Qasim Ijaz]

In mii_nway_restart() during the line:

        bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);

The code attempts to call mii->mdio_read which is ch9200_mdio_read().

ch9200_mdio_read() utilises a local buffer, which is initialised
with control_read():

        unsigned char buff[2];

However buff is conditionally initialised inside control_read():

        if (err == size) {
                memcpy(data, buf, size);
        }

If the condition of "err == size" is not met, then buff remains
uninitialised. Once this happens the uninitialised buff is accessed
and returned during ch9200_mdio_read():

        return (buff[0] | buff[1] << 8);

The problem stems from the fact that ch9200_mdio_read() ignores the
return value of control_read(), leading to uinit-access of buff.

To fix this we should check the return value of control_read()
and return early on error.

Reported-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=3361c2d6f78a3e0892f9
Tested-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Cc: stable@vger.kernel.org
Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
---
 drivers/net/mii.c        | 2 ++
 drivers/net/usb/ch9200.c | 7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mii.c b/drivers/net/mii.c
index 37bc3131d31a..e305bf0f1d04 100644
--- a/drivers/net/mii.c
+++ b/drivers/net/mii.c
@@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
 
 	/* if autoneg is off, it's an error */
 	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
+	if (bmcr < 0)
+		return bmcr;
 
 	if (bmcr & BMCR_ANENABLE) {
 		bmcr |= BMCR_ANRESTART;
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
index f69d9b902da0..a206ffa76f1b 100644
--- a/drivers/net/usb/ch9200.c
+++ b/drivers/net/usb/ch9200.c
@@ -178,6 +178,7 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc)
 {
 	struct usbnet *dev = netdev_priv(netdev);
 	unsigned char buff[2];
+	int ret;
 
 	netdev_dbg(netdev, "%s phy_id:%02x loc:%02x\n",
 		   __func__, phy_id, loc);
@@ -185,8 +186,10 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc)
 	if (phy_id != 0)
 		return -ENODEV;
 
-	control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02,
-		     CONTROL_TIMEOUT_MS);
+	ret = control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02,
+			   CONTROL_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
 
 	return (buff[0] | buff[1] << 8);
 }
-- 
2.39.5

[PATCH 2/4] net: ch9200: remove extraneous return in control_write() to propagate failures

[Qasim Ijaz]

The control_write() function sets err to -EINVAL however there 
is an incorrectly placed 'return 0' statement after it which stops 
the propogation of the error.

Fix this issue by removing the 'return 0'.

Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
---
 drivers/net/usb/ch9200.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
index a206ffa76f1b..3a81e9e96fd3 100644
--- a/drivers/net/usb/ch9200.c
+++ b/drivers/net/usb/ch9200.c
@@ -168,8 +168,6 @@ static int control_write(struct usbnet *dev, unsigned char request,
 		err = -EINVAL;
 	kfree(buf);
 
-	return 0;
-
 err_out:
 	return err;
 }
-- 
2.39.5

[PATCH 3/4] net: ch9200: improve error handling in get_mac_address()

[Qasim Ijaz]

The get_mac_address() function has an issue where it does not 
directly check the return value of each control_read(), instead 
it sums up the return values and checks them all at the end
which means if any call to control_read() fails the function just
continues on.

Handle this by validating the return value of each call and fail fast
and early instead of continuing.

Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
---
 drivers/net/usb/ch9200.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
index 3a81e9e96fd3..a910aea0febe 100644
--- a/drivers/net/usb/ch9200.c
+++ b/drivers/net/usb/ch9200.c
@@ -304,24 +304,27 @@ static int ch9200_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 
 static int get_mac_address(struct usbnet *dev, unsigned char *data)
 {
-	int err = 0;
 	unsigned char mac_addr[0x06];
-	int rd_mac_len = 0;
+	int rd_mac_len;
 
 	netdev_dbg(dev->net, "%s:\n\tusbnet VID:%0x PID:%0x\n", __func__,
 		   le16_to_cpu(dev->udev->descriptor.idVendor),
 		   le16_to_cpu(dev->udev->descriptor.idProduct));
 
-	memset(mac_addr, 0, sizeof(mac_addr));
-	rd_mac_len = control_read(dev, REQUEST_READ, 0,
-				  MAC_REG_STATION_L, mac_addr, 0x02,
-				  CONTROL_TIMEOUT_MS);
-	rd_mac_len += control_read(dev, REQUEST_READ, 0, MAC_REG_STATION_M,
-				   mac_addr + 2, 0x02, CONTROL_TIMEOUT_MS);
-	rd_mac_len += control_read(dev, REQUEST_READ, 0, MAC_REG_STATION_H,
-				   mac_addr + 4, 0x02, CONTROL_TIMEOUT_MS);
-	if (rd_mac_len != ETH_ALEN)
-		err = -EINVAL;
+	rd_mac_len = control_read(dev, REQUEST_READ, 0, MAC_REG_STATION_L,
+				  mac_addr, 0x02, CONTROL_TIMEOUT_MS);
+	if (rd_mac_len < 0)
+		return rd_mac_len;
+
+	rd_mac_len = control_read(dev, REQUEST_READ, 0, MAC_REG_STATION_M,
+				  mac_addr + 2, 0x02, CONTROL_TIMEOUT_MS);
+	if (rd_mac_len < 0)
+		return rd_mac_len;
+
+	rd_mac_len = control_read(dev, REQUEST_READ, 0, MAC_REG_STATION_H,
+				  mac_addr + 4, 0x02, CONTROL_TIMEOUT_MS);
+	if (rd_mac_len < 0)
+		return rd_mac_len;
 
 	data[0] = mac_addr[5];
 	data[1] = mac_addr[4];
@@ -330,7 +333,7 @@ static int get_mac_address(struct usbnet *dev, unsigned char *data)
 	data[4] = mac_addr[1];
 	data[5] = mac_addr[0];
 
-	return err;
+	return 0;
 }
 
 static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf)
@@ -386,6 +389,9 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf)
 			       CONTROL_TIMEOUT_MS);
 
 	retval = get_mac_address(dev, addr);
+	if (retval < 0)
+		return retval;
+
 	eth_hw_addr_set(dev->net, addr);
 
 	return retval;
-- 
2.39.5

[PATCH 4/4] net: ch9200: add error handling in ch9200_bind()

[Qasim Ijaz]

The ch9200_bind() function has no error handling for any
control_write() calls.

Fix this by checking if any control_write() call fails and 
propagate the error to the caller.

Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
---
 drivers/net/usb/ch9200.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
index a910aea0febe..01ed37e9f725 100644
--- a/drivers/net/usb/ch9200.c
+++ b/drivers/net/usb/ch9200.c
@@ -338,12 +338,12 @@ static int get_mac_address(struct usbnet *dev, unsigned char *data)
 
 static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf)
 {
-	int retval = 0;
+	int retval;
 	unsigned char data[2];
 	u8 addr[ETH_ALEN];
 
 	retval = usbnet_get_endpoints(dev, intf);
-	if (retval)
+	if (retval < 0)
 		return retval;
 
 	dev->mii.dev = dev->net;
@@ -361,32 +361,44 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf)
 	data[1] = 0x0F;
 	retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_THRESHOLD, data,
 			       0x02, CONTROL_TIMEOUT_MS);
+	if (retval < 0)
+		return retval;
 
 	data[0] = 0xA0;
 	data[1] = 0x90;
 	retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_FIFO_DEPTH, data,
 			       0x02, CONTROL_TIMEOUT_MS);
+	if (retval < 0)
+		return retval;
 
 	data[0] = 0x30;
 	data[1] = 0x00;
 	retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_PAUSE, data,
 			       0x02, CONTROL_TIMEOUT_MS);
+	if (retval < 0)
+		return retval;
 
 	data[0] = 0x17;
 	data[1] = 0xD8;
 	retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_FLOW_CONTROL,
 			       data, 0x02, CONTROL_TIMEOUT_MS);
+	if (retval < 0)
+		return retval;
 
 	/* Undocumented register */
 	data[0] = 0x01;
 	data[1] = 0x00;
 	retval = control_write(dev, REQUEST_WRITE, 0, 254, data, 0x02,
 			       CONTROL_TIMEOUT_MS);
+	if (retval < 0)
+		return retval;
 
 	data[0] = 0x5F;
 	data[1] = 0x0D;
 	retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_CTRL, data, 0x02,
 			       CONTROL_TIMEOUT_MS);
+	if (retval < 0)
+		return retval;
 
 	retval = get_mac_address(dev, addr);
 	if (retval < 0)
@@ -394,7 +406,7 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf)
 
 	eth_hw_addr_set(dev->net, addr);
 
-	return retval;
+	return 0;
 }
 
 static const struct driver_info ch9200_info = {
-- 
2.39.5

Re: [PATCH 3/4] net: ch9200: improve error handling in get_mac_address()

[Markus Elfring]

> The get_mac_address() function has an issue where it does not
> directly check the return value of each control_read(), instead
> it sums up the return values and checks them all at the end
> which means if any call to control_read() fails the function just
> continues on.
…

How does such a change description fit to the additional adjustment
of the implementation for the function “ch9200_bind”?
Would another update step become relevant here?

Regards,
Markus

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Simon Horman]

On Wed, Mar 19, 2025 at 11:21:53AM +0000, Qasim Ijaz wrote:
> In mii_nway_restart() during the line:
> 
>         bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
> 
> The code attempts to call mii->mdio_read which is ch9200_mdio_read().
> 
> ch9200_mdio_read() utilises a local buffer, which is initialised
> with control_read():
> 
>         unsigned char buff[2];
> 
> However buff is conditionally initialised inside control_read():
> 
>         if (err == size) {
>                 memcpy(data, buf, size);
>         }
> 
> If the condition of "err == size" is not met, then buff remains
> uninitialised. Once this happens the uninitialised buff is accessed
> and returned during ch9200_mdio_read():
> 
>         return (buff[0] | buff[1] << 8);
> 
> The problem stems from the fact that ch9200_mdio_read() ignores the
> return value of control_read(), leading to uinit-access of buff.
> 
> To fix this we should check the return value of control_read()
> and return early on error.
> 
> Reported-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=3361c2d6f78a3e0892f9
> Tested-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
> Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH 2/4] net: ch9200: remove extraneous return in control_write() to propagate failures

[Simon Horman]

On Wed, Mar 19, 2025 at 11:21:54AM +0000, Qasim Ijaz wrote:
> The control_write() function sets err to -EINVAL however there 
> is an incorrectly placed 'return 0' statement after it which stops 
> the propogation of the error.
> 
> Fix this issue by removing the 'return 0'.
> 
> Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH 3/4] net: ch9200: improve error handling in get_mac_address()

[Simon Horman]

On Wed, Mar 19, 2025 at 11:21:55AM +0000, Qasim Ijaz wrote:
> The get_mac_address() function has an issue where it does not 
> directly check the return value of each control_read(), instead 
> it sums up the return values and checks them all at the end
> which means if any call to control_read() fails the function just
> continues on.
> 
> Handle this by validating the return value of each call and fail fast
> and early instead of continuing.
> 
> Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH 4/4] net: ch9200: add error handling in ch9200_bind()

[Simon Horman]

On Wed, Mar 19, 2025 at 11:21:56AM +0000, Qasim Ijaz wrote:
> The ch9200_bind() function has no error handling for any
> control_write() calls.
> 
> Fix this by checking if any control_write() call fails and 
> propagate the error to the caller.
> 
> Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH 4/4] net: ch9200: add error handling in ch9200_bind()

[Markus Elfring]

…
> Fix this by checking if any control_write() call fails and
> propagate the error to the caller.

Another wording suggestion:
Thus check if any control_write() call failed and propagate the error
to the caller.



Can it eventually be helpful to transform six control_write() calls
into a loop?

Regards,
Markus

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Jakub Kicinski]

On Wed, 19 Mar 2025 11:21:53 +0000 Qasim Ijaz wrote:
> --- a/drivers/net/mii.c
> +++ b/drivers/net/mii.c
> @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
>  
>  	/* if autoneg is off, it's an error */
>  	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
> +	if (bmcr < 0)
> +		return bmcr;
>  
>  	if (bmcr & BMCR_ANENABLE) {
>  		bmcr |= BMCR_ANRESTART;

We error check just one mdio_read() but there's a whole bunch of them
in this file. What's the expected behavior then? Are all of them buggy?

This patch should be split into core and driver parts.
-- 
pw-bot: cr

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Qasim Ijaz]

On Tue, Mar 25, 2025 at 06:33:07AM -0700, Jakub Kicinski wrote:
> On Wed, 19 Mar 2025 11:21:53 +0000 Qasim Ijaz wrote:
> > --- a/drivers/net/mii.c
> > +++ b/drivers/net/mii.c
> > @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
> >  
> >  	/* if autoneg is off, it's an error */
> >  	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
> > +	if (bmcr < 0)
> > +		return bmcr;
> >  
> >  	if (bmcr & BMCR_ANENABLE) {
> >  		bmcr |= BMCR_ANRESTART;
> 
> We error check just one mdio_read() but there's a whole bunch of them
> in this file. What's the expected behavior then? Are all of them buggy?
>
 
Hi Jakub
    
Apologies for my delayed response, I had another look at this and I
think my patch may be off a bit. You are correct that there are multiple
mdio_read() calls and looking at the mii.c file we can see that calls to
functions like mdio_read (and a lot of others) dont check return values.
  
So in light of this I think a better patch would be to not edit the 
mii.c file at all and just make ch9200_mdio_read return 0 on     
error. This way if mdio_read fails and 0 is returned, the         
check for "bmcr & BMCR_ANENABLE" won't be triggered and mii_nway_restart
will just return 0 and end. If we return a negative on error it may
contain the exact bit the function checks.

Similiar to this patch:
<https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=c68b2c9eba38>

If this sounds good, should i send another patch series with all the
changes? 

> This patch should be split into core and driver parts.
> -- 
> pw-bot: cr

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Jakub Kicinski]

On Thu, 10 Apr 2025 23:15:23 +0100 Qasim Ijaz wrote:
> Apologies for my delayed response, I had another look at this and I
> think my patch may be off a bit. You are correct that there are multiple
> mdio_read() calls and looking at the mii.c file we can see that calls to
> functions like mdio_read (and a lot of others) dont check return values.
>   
> So in light of this I think a better patch would be to not edit the 
> mii.c file at all and just make ch9200_mdio_read return 0 on     
> error. This way if mdio_read fails and 0 is returned, the         
> check for "bmcr & BMCR_ANENABLE" won't be triggered and mii_nway_restart
> will just return 0 and end. If we return a negative on error it may
> contain the exact bit the function checks.
> 
> Similiar to this patch:
> <https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=c68b2c9eba38>
> 
> If this sounds good, should i send another patch series with all the
> changes? 

SG

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Andrew Lunn]

On Thu, Apr 10, 2025 at 11:15:23PM +0100, Qasim Ijaz wrote:
> On Tue, Mar 25, 2025 at 06:33:07AM -0700, Jakub Kicinski wrote:
> > On Wed, 19 Mar 2025 11:21:53 +0000 Qasim Ijaz wrote:
> > > --- a/drivers/net/mii.c
> > > +++ b/drivers/net/mii.c
> > > @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
> > >  
> > >  	/* if autoneg is off, it's an error */
> > >  	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
> > > +	if (bmcr < 0)
> > > +		return bmcr;
> > >  
> > >  	if (bmcr & BMCR_ANENABLE) {
> > >  		bmcr |= BMCR_ANRESTART;
> > 
> > We error check just one mdio_read() but there's a whole bunch of them
> > in this file. What's the expected behavior then? Are all of them buggy?
> >
>  
> Hi Jakub
>     
> Apologies for my delayed response, I had another look at this and I
> think my patch may be off a bit. You are correct that there are multiple
> mdio_read() calls and looking at the mii.c file we can see that calls to
> functions like mdio_read (and a lot of others) dont check return values.
>   
> So in light of this I think a better patch would be to not edit the 
> mii.c file at all and just make ch9200_mdio_read return 0 on     
> error.

Do you actually have one of these devices? If you do have, an even
better change would be to throwaway the mii code and swap to phylib
and an MDIO bus. You can probably follow smsc95xx.c.

	Andrew

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Qasim Ijaz]

On Fri, Apr 11, 2025 at 03:12:06AM +0200, Andrew Lunn wrote:
> On Thu, Apr 10, 2025 at 11:15:23PM +0100, Qasim Ijaz wrote:
> > On Tue, Mar 25, 2025 at 06:33:07AM -0700, Jakub Kicinski wrote:
> > > On Wed, 19 Mar 2025 11:21:53 +0000 Qasim Ijaz wrote:
> > > > --- a/drivers/net/mii.c
> > > > +++ b/drivers/net/mii.c
> > > > @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
> > > >  
> > > >  	/* if autoneg is off, it's an error */
> > > >  	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
> > > > +	if (bmcr < 0)
> > > > +		return bmcr;
> > > >  
> > > >  	if (bmcr & BMCR_ANENABLE) {
> > > >  		bmcr |= BMCR_ANRESTART;
> > > 
> > > We error check just one mdio_read() but there's a whole bunch of them
> > > in this file. What's the expected behavior then? Are all of them buggy?
> > >
> >  
> > Hi Jakub
> >     
> > Apologies for my delayed response, I had another look at this and I
> > think my patch may be off a bit. You are correct that there are multiple
> > mdio_read() calls and looking at the mii.c file we can see that calls to
> > functions like mdio_read (and a lot of others) dont check return values.
> >   
> > So in light of this I think a better patch would be to not edit the 
> > mii.c file at all and just make ch9200_mdio_read return 0 on     
> > error.
> 
> Do you actually have one of these devices? If you do have, an even
> better change would be to throwaway the mii code and swap to phylib
> and an MDIO bus. You can probably follow smsc95xx.c.
> 

Hi Andrew,

Thanks for the suggestion. I don't have one of these devices at the moment.
If in the future if I do I will definitely explore the suggestion more.

Regards,
Qasim

> 	Andrew