From mboxrd@z Thu Jan 1 00:00:00 1970 From: andriy.shevchenko@linux.intel.com (Andy Shevchenko) Date: Thu, 14 Jan 2016 11:43:55 +0200 Subject: [PATCH v2 next-next] net: hns: enet specifies a reference to dsaf In-Reply-To: <56970F48.9060504@huawei.com> References: <1452654880-28980-1-git-send-email-yankejian@huawei.com> <56970F48.9060504@huawei.com> Message-ID: <1452764635.2521.28.camel@linux.intel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, 2016-01-14 at 11:00 +0800, Yisen Zhuang wrote: > > ? 2016/1/13 11:14, Kejian Yan ??: > > This patch replace the assoication between dsaf and enet from > > string > > matching to object reference. It requires the DTS to be updated > > within > > BIOS. Thanks god it can be done for all released boards. > > > > Hi kejian, > > This patch is fine to me. There are few thing below. >? > > --- a/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c > > +++ b/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c > > @@ -847,6 +847,7 @@ static struct hnae_ae_ops hns_dsaf_ops = { > > ?int hns_dsaf_ae_init(struct dsaf_device *dsaf_dev) > > ?{ > > ? struct hnae_ae_dev *ae_dev = &dsaf_dev->ae_dev; > > + static atomic_t id = ATOMIC_INIT(-1); > > ? > > ? switch (dsaf_dev->dsaf_ver) { > > ? case AE_VERSION_1: > > @@ -858,6 +859,9 @@ int hns_dsaf_ae_init(struct dsaf_device > > *dsaf_dev) > > ? default: > > ? break; > > ? } > > + > > + snprintf(ae_dev->name, AE_NAME_SIZE, "%s%d", > > DSAF_DEVICE_NAME, > > + ?(int)atomic_inc_return(&id)); If you bind/unbind device enough times you may get an overflow and end up with name of existing device (if you have 1+ of them in the system). To avoid such situation better to use IDA/IDR framework. > >? > > --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c > > +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c > > @@ -1802,7 +1802,7 @@ static int hns_nic_try_get_ae(struct > > net_device *ndev) > > ? int ret; > > ? > > ? h = hnae_get_handle(&priv->netdev->dev, > > - ????priv->ae_name, priv->port_id, NULL); > > + ????priv->ae_node, priv->port_id, NULL); > > ? if (IS_ERR_OR_NULL(h)) { > > ? ret = PTR_ERR(h); > > ? dev_dbg(priv->dev, "has not handle, register > > notifier!\n"); > > @@ -1880,9 +1880,12 @@ static int hns_nic_dev_probe(struct > > platform_device *pdev) > > ? else > > ? priv->enet_ver = AE_VERSION_2; > > ? > > - ret = of_property_read_string(node, "ae-name", &priv- > > >ae_name); > > - if (ret) > > - goto out_read_string_fail; (1) > > + priv->ae_node = (void *)of_parse_phandle(node, "ae- > > handle", 0); > > + if (IS_ERR_OR_NULL(priv->ae_node)) { > > + ret = PTR_ERR(priv->ae_node); > > + dev_err(dev, "not find ae-handle\n"); > > + goto out_read_handle_fai; (2) > > + } > > ? > > ? ret = of_property_read_u32(node, "port-id", &priv- > > >port_id); > > ? if (ret) > > @@ -1945,6 +1948,8 @@ static int hns_nic_dev_probe(struct > > platform_device *pdev) > > ? > > ?out_notify_fail: > > ? (void)cancel_work_sync(&priv->service_task); > > +out_read_handle_fai: > > + Redundant line > > ?out_read_string_fail: Leftover? (see (1) and (2) ) -- Andy Shevchenko Intel Finland Oy From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Shevchenko Subject: Re: [PATCH v2 next-next] net: hns: enet specifies a reference to dsaf Date: Thu, 14 Jan 2016 11:43:55 +0200 Message-ID: <1452764635.2521.28.camel@linux.intel.com> References: <1452654880-28980-1-git-send-email-yankejian@huawei.com> <56970F48.9060504@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <56970F48.9060504-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Yisen Zhuang , Kejian Yan , davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, pawel.moll-5wv7dgnIgG8@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org, galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, catalin.marinas-5wv7dgnIgG8@public.gmane.org, will.deacon-5wv7dgnIgG8@public.gmane.org, huangdaode-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org, liguozhu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org, arnd-r2nGTMty4D4@public.gmane.org, fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, salil.mehta-hv44wF8Li93QT0dZR+AlfA@public.gmane.org, lisheng011-hv44wF8Li93QT0dZR+AlfA@public.gmane.org Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linuxarm-hv44wF8Li93QT0dZR+AlfA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org On Thu, 2016-01-14 at 11:00 +0800, Yisen Zhuang wrote: >=20 > =E5=9C=A8 2016/1/13 11:14, Kejian Yan =E5=86=99=E9=81=93: > > This patch replace the assoication between dsaf and enet from > > string > > matching to object reference. It requires the DTS to be updated > > within > > BIOS. Thanks god it can be done for all released boards. > >=20 >=20 > Hi kejian, >=20 > This patch is fine to me. There are few thing below. >=C2=A0 > > --- a/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c > > +++ b/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c > > @@ -847,6 +847,7 @@ static struct hnae_ae_ops hns_dsaf_ops =3D { > > =C2=A0int hns_dsaf_ae_init(struct dsaf_device *dsaf_dev) > > =C2=A0{ > > =C2=A0 struct hnae_ae_dev *ae_dev =3D &dsaf_dev->ae_dev; > > + static atomic_t id =3D ATOMIC_INIT(-1); > > =C2=A0 > > =C2=A0 switch (dsaf_dev->dsaf_ver) { > > =C2=A0 case AE_VERSION_1: > > @@ -858,6 +859,9 @@ int hns_dsaf_ae_init(struct dsaf_device > > *dsaf_dev) > > =C2=A0 default: > > =C2=A0 break; > > =C2=A0 } > > + > > + snprintf(ae_dev->name, AE_NAME_SIZE, "%s%d", > > DSAF_DEVICE_NAME, > > + =C2=A0(int)atomic_inc_return(&id)); If you bind/unbind device enough times you may get an overflow and end up with name of existing device (if you have 1+ of them in the system). To avoid such situation better to use IDA/IDR framework. > >=C2=A0 > > --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c > > +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c > > @@ -1802,7 +1802,7 @@ static int hns_nic_try_get_ae(struct > > net_device *ndev) > > =C2=A0 int ret; > > =C2=A0 > > =C2=A0 h =3D hnae_get_handle(&priv->netdev->dev, > > - =C2=A0=C2=A0=C2=A0=C2=A0priv->ae_name, priv->port_id, NULL); > > + =C2=A0=C2=A0=C2=A0=C2=A0priv->ae_node, priv->port_id, NULL); > > =C2=A0 if (IS_ERR_OR_NULL(h)) { > > =C2=A0 ret =3D PTR_ERR(h); > > =C2=A0 dev_dbg(priv->dev, "has not handle, register > > notifier!\n"); > > @@ -1880,9 +1880,12 @@ static int hns_nic_dev_probe(struct > > platform_device *pdev) > > =C2=A0 else > > =C2=A0 priv->enet_ver =3D AE_VERSION_2; > > =C2=A0 > > - ret =3D of_property_read_string(node, "ae-name", &priv- > > >ae_name); > > - if (ret) > > - goto out_read_string_fail; (1) > > + priv->ae_node =3D (void *)of_parse_phandle(node, "ae- > > handle", 0); > > + if (IS_ERR_OR_NULL(priv->ae_node)) { > > + ret =3D PTR_ERR(priv->ae_node); > > + dev_err(dev, "not find ae-handle\n"); > > + goto out_read_handle_fai; (2) > > + } > > =C2=A0 > > =C2=A0 ret =3D of_property_read_u32(node, "port-id", &priv- > > >port_id); > > =C2=A0 if (ret) > > @@ -1945,6 +1948,8 @@ static int hns_nic_dev_probe(struct > > platform_device *pdev) > > =C2=A0 > > =C2=A0out_notify_fail: > > =C2=A0 (void)cancel_work_sync(&priv->service_task); > > +out_read_handle_fai: > > + Redundant line > > =C2=A0out_read_string_fail: Leftover? (see (1) and (2) ) --=20 Andy Shevchenko Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe devicetree" i= n the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753304AbcANJoJ (ORCPT ); Thu, 14 Jan 2016 04:44:09 -0500 Received: from mga04.intel.com ([192.55.52.120]:4776 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752448AbcANJnd (ORCPT ); Thu, 14 Jan 2016 04:43:33 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,293,1449561600"; d="scan'208";a="860318008" Message-ID: <1452764635.2521.28.camel@linux.intel.com> Subject: Re: [PATCH v2 next-next] net: hns: enet specifies a reference to dsaf From: Andy Shevchenko To: Yisen Zhuang , Kejian Yan , davem@davemloft.net, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, catalin.marinas@arm.com, will.deacon@arm.com, huangdaode@hisilicon.com, liguozhu@huawei.com, arnd@arndb.de, fengguang.wu@intel.com, salil.mehta@huawei.com, lisheng011@huawei.com Cc: devicetree@vger.kernel.org, netdev@vger.kernel.org, linuxarm@huawei.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Date: Thu, 14 Jan 2016 11:43:55 +0200 In-Reply-To: <56970F48.9060504@huawei.com> References: <1452654880-28980-1-git-send-email-yankejian@huawei.com> <56970F48.9060504@huawei.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2016-01-14 at 11:00 +0800, Yisen Zhuang wrote: > > 在 2016/1/13 11:14, Kejian Yan 写道: > > This patch replace the assoication between dsaf and enet from > > string > > matching to object reference. It requires the DTS to be updated > > within > > BIOS. Thanks god it can be done for all released boards. > > > > Hi kejian, > > This patch is fine to me. There are few thing below. >  > > --- a/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c > > +++ b/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c > > @@ -847,6 +847,7 @@ static struct hnae_ae_ops hns_dsaf_ops = { > >  int hns_dsaf_ae_init(struct dsaf_device *dsaf_dev) > >  { > >   struct hnae_ae_dev *ae_dev = &dsaf_dev->ae_dev; > > + static atomic_t id = ATOMIC_INIT(-1); > >   > >   switch (dsaf_dev->dsaf_ver) { > >   case AE_VERSION_1: > > @@ -858,6 +859,9 @@ int hns_dsaf_ae_init(struct dsaf_device > > *dsaf_dev) > >   default: > >   break; > >   } > > + > > + snprintf(ae_dev->name, AE_NAME_SIZE, "%s%d", > > DSAF_DEVICE_NAME, > > +  (int)atomic_inc_return(&id)); If you bind/unbind device enough times you may get an overflow and end up with name of existing device (if you have 1+ of them in the system). To avoid such situation better to use IDA/IDR framework. > >  > > --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c > > +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c > > @@ -1802,7 +1802,7 @@ static int hns_nic_try_get_ae(struct > > net_device *ndev) > >   int ret; > >   > >   h = hnae_get_handle(&priv->netdev->dev, > > -     priv->ae_name, priv->port_id, NULL); > > +     priv->ae_node, priv->port_id, NULL); > >   if (IS_ERR_OR_NULL(h)) { > >   ret = PTR_ERR(h); > >   dev_dbg(priv->dev, "has not handle, register > > notifier!\n"); > > @@ -1880,9 +1880,12 @@ static int hns_nic_dev_probe(struct > > platform_device *pdev) > >   else > >   priv->enet_ver = AE_VERSION_2; > >   > > - ret = of_property_read_string(node, "ae-name", &priv- > > >ae_name); > > - if (ret) > > - goto out_read_string_fail; (1) > > + priv->ae_node = (void *)of_parse_phandle(node, "ae- > > handle", 0); > > + if (IS_ERR_OR_NULL(priv->ae_node)) { > > + ret = PTR_ERR(priv->ae_node); > > + dev_err(dev, "not find ae-handle\n"); > > + goto out_read_handle_fai; (2) > > + } > >   > >   ret = of_property_read_u32(node, "port-id", &priv- > > >port_id); > >   if (ret) > > @@ -1945,6 +1948,8 @@ static int hns_nic_dev_probe(struct > > platform_device *pdev) > >   > >  out_notify_fail: > >   (void)cancel_work_sync(&priv->service_task); > > +out_read_handle_fai: > > + Redundant line > >  out_read_string_fail: Leftover? (see (1) and (2) ) -- Andy Shevchenko Intel Finland Oy