From: Haiyang Zhang <haiyangz@microsoft.com>
To: Dexuan Cui <decui@microsoft.com>,
"davem@davemloft.net" <davem@davemloft.net>,
"kuba@kernel.org" <kuba@kernel.org>,
"gustavoars@kernel.org" <gustavoars@kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Cc: KY Srinivasan <kys@microsoft.com>,
"stephen@networkplumber.org" <stephen@networkplumber.org>,
"wei.liu@kernel.org" <wei.liu@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
Shachar Raindel <shacharr@microsoft.com>,
Paul Rosswurm <paulros@microsoft.com>,
"olaf@aepfle.de" <olaf@aepfle.de>, vkuznets <vkuznets@redhat.com>
Subject: RE: [PATCH net-next 4/4] net: mana: Support hibernation and kexec
Date: Mon, 1 Nov 2021 15:12:22 +0000 [thread overview]
Message-ID: <BN8PR21MB128460B5BE376A9E05D43418CA8A9@BN8PR21MB1284.namprd21.prod.outlook.com> (raw)
In-Reply-To: <BYAPR21MB1270A16AA0BF1A302BABCB3FBF8A9@BYAPR21MB1270.namprd21.prod.outlook.com>
> -----Original Message-----
> From: Dexuan Cui <decui@microsoft.com>
> Sent: Monday, November 1, 2021 3:03 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>; davem@davemloft.net;
> kuba@kernel.org; gustavoars@kernel.org; netdev@vger.kernel.org
> Cc: KY Srinivasan <kys@microsoft.com>; stephen@networkplumber.org;
> wei.liu@kernel.org; linux-kernel@vger.kernel.org; linux-
> hyperv@vger.kernel.org; Shachar Raindel <shacharr@microsoft.com>; Paul
> Rosswurm <paulros@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>
> Subject: RE: [PATCH net-next 4/4] net: mana: Support hibernation and
> kexec
>
> > From: Haiyang Zhang <haiyangz@microsoft.com>
> > Sent: Saturday, October 30, 2021 8:55 AM
> > >
> > > @@ -1844,44 +1845,70 @@ int mana_probe(struct gdma_dev *gd)
> > > if (err)
> > > return err;
> > >
> > > - ac = kzalloc(sizeof(*ac), GFP_KERNEL);
> > > - if (!ac)
> > > - return -ENOMEM;
> > > + if (!resuming) {
> > > + ac = kzalloc(sizeof(*ac), GFP_KERNEL);
> > > + if (!ac)
> > > + return -ENOMEM;
> > >
> > > - ac->gdma_dev = gd;
> > > - ac->num_ports = 1;
> > > - gd->driver_data = ac;
> > > + ac->gdma_dev = gd;
> > > + gd->driver_data = ac;
> > > + }
> > >
> > > err = mana_create_eq(ac);
> > > if (err)
> > > goto out;
> > >
> > > err = mana_query_device_cfg(ac, MANA_MAJOR_VERSION,
> > > MANA_MINOR_VERSION,
> > > - MANA_MICRO_VERSION, &ac->num_ports);
> > > + MANA_MICRO_VERSION, &num_ports);
> > > if (err)
> > > goto out;
> > >
> > > + if (!resuming) {
> > > + ac->num_ports = num_ports;
> > > + } else {
> > > + if (ac->num_ports != num_ports) {
> > > + dev_err(dev, "The number of vPorts changed: %d->%d\n",
> > > + ac->num_ports, num_ports);
> > > + err = -EPROTO;
> > > + goto out;
> > > + }
> > > + }
> > > +
> > > + if (ac->num_ports == 0)
> > > + dev_err(dev, "Failed to detect any vPort\n");
> > > +
> > > if (ac->num_ports > MAX_PORTS_IN_MANA_DEV)
> > > ac->num_ports = MAX_PORTS_IN_MANA_DEV;
> > >
> > > - for (i = 0; i < ac->num_ports; i++) {
> > > - err = mana_probe_port(ac, i, &ac->ports[i]);
> > > - if (err)
> > > - break;
> > > + if (!resuming) {
> > > + for (i = 0; i < ac->num_ports; i++) {
> > > + err = mana_probe_port(ac, i, &ac->ports[i]);
> > > + if (err)
> > > + break;
> > > + }
> > > + } else {
> > > + for (i = 0; i < ac->num_ports; i++) {
> > > + rtnl_lock();
> > > + err = mana_attach(ac->ports[i]);
> > > + rtnl_unlock();
> > > + if (err)
> > > + break;
> > > + }
> > > }
> > > out:
> > > if (err)
> > > - mana_remove(gd);
> > > + mana_remove(gd, false);
> >
> > The "goto out" can happen in both resuming true/false cases,
> > should the error handling path deal with the two cases
> > differently?
>
> Let me make the below change in v2. Please let me know
> if any further change is needed:
>
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -1850,8 +1850,10 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
>
> if (!resuming) {
> ac = kzalloc(sizeof(*ac), GFP_KERNEL);
> - if (!ac)
> - return -ENOMEM;
> + if (!ac) {
> + err = -ENOMEM;
> + goto out;
> + }
>
> ac->gdma_dev = gd;
> gd->driver_data = ac;
> @@ -1872,8 +1874,8 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
> if (ac->num_ports != num_ports) {
> dev_err(dev, "The number of vPorts changed: %d-
> >%d\n",
> ac->num_ports, num_ports);
> - err = -EPROTO;
> - goto out;
> + /* It's unsafe to proceed. */
> + return -EPROTO;
> }
> }
>
> @@ -1886,22 +1888,36 @@ int mana_probe(struct gdma_dev *gd, bool
> resuming)
> if (!resuming) {
> for (i = 0; i < ac->num_ports; i++) {
> err = mana_probe_port(ac, i, &ac->ports[i]);
> - if (err)
> - break;
> + if (err) {
> + dev_err(dev, "Failed to probe
> vPort %u: %d\n",
> + i, err);
> + goto out;
> + }
> }
> } else {
> for (i = 0; i < ac->num_ports; i++) {
> rtnl_lock();
> err = mana_attach(ac->ports[i]);
> rtnl_unlock();
> - if (err)
> - break;
> +
> + if (err) {
> + netdev_err(ac->ports[i],
> + "Failed to resume
> vPort %u: %d\n",
> + i, err);
> + return err;
> + }
> }
> }
> +
> + return 0;
> out:
> - if (err)
> - mana_remove(gd, false);
> + /* In the resuming path, it's safer to leave the device in the
> failed
> + * state than try to invoke mana_detach().
> + */
> + if (resuming)
> + return err;
>
> + mana_remove(gd, false);
> return err;
> }
>
> @@ -1919,7 +1935,7 @@ void mana_remove(struct gdma_dev *gd, bool
> suspending)
> if (!ndev) {
> if (i == 0)
> dev_err(dev, "No net device to
> remove\n");
> - goto out;
> + break;
> }
>
> /* All cleanup actions should stay after rtnl_lock(),
> otherwise
>
> For your easy reviewing, the new code of the function in v2 will be:
>
> int mana_probe(struct gdma_dev *gd, bool resuming)
> {
> struct gdma_context *gc = gd->gdma_context;
> struct mana_context *ac = gd->driver_data;
> struct device *dev = gc->dev;
> u16 num_ports = 0;
> int err;
> int i;
>
> dev_info(dev,
> "Microsoft Azure Network Adapter protocol
> version: %d.%d.%d\n",
> MANA_MAJOR_VERSION, MANA_MINOR_VERSION,
> MANA_MICRO_VERSION);
>
> err = mana_gd_register_device(gd);
> if (err)
> return err;
>
> if (!resuming) {
> ac = kzalloc(sizeof(*ac), GFP_KERNEL);
> if (!ac) {
> err = -ENOMEM;
> goto out;
> }
>
> ac->gdma_dev = gd;
> gd->driver_data = ac;
> }
>
> err = mana_create_eq(ac);
> if (err)
> goto out;
>
> err = mana_query_device_cfg(ac, MANA_MAJOR_VERSION,
> MANA_MINOR_VERSION,
> MANA_MICRO_VERSION, &num_ports);
> if (err)
> goto out;
>
> if (!resuming) {
> ac->num_ports = num_ports;
> } else {
> if (ac->num_ports != num_ports) {
> dev_err(dev, "The number of vPorts changed: %d-
> >%d\n",
> ac->num_ports, num_ports);
> /* It's unsafe to proceed. */
> return -EPROTO;
> }
> }
>
> if (ac->num_ports == 0)
> dev_err(dev, "Failed to detect any vPort\n");
>
> if (ac->num_ports > MAX_PORTS_IN_MANA_DEV)
> ac->num_ports = MAX_PORTS_IN_MANA_DEV;
>
> if (!resuming) {
> for (i = 0; i < ac->num_ports; i++) {
> err = mana_probe_port(ac, i, &ac->ports[i]);
> if (err) {
> dev_err(dev, "Failed to probe
> vPort %u: %d\n",
> i, err);
> goto out;
> }
> }
> } else {
> for (i = 0; i < ac->num_ports; i++) {
> rtnl_lock();
> err = mana_attach(ac->ports[i]);
> rtnl_unlock();
>
> if (err) {
> netdev_err(ac->ports[i],
> "Failed to resume
> vPort %u: %d\n",
> i, err);
> return err;
> }
> }
> }
>
> return 0;
> out:
> /* In the resuming path, it's safer to leave the device in the
> failed
> * state than try to invoke mana_detach().
> */
> if (resuming)
> return err;
>
> mana_remove(gd, false);
> return err;
> }
The new code looks good!
Thanks,
- Haiyang
next prev parent reply other threads:[~2021-11-01 15:12 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-30 0:54 [PATCH net-next 0/4] net: mana: some misc patches Dexuan Cui
2021-10-30 0:54 ` [PATCH net-next 1/4] net: mana: Fix the netdev_err()'s vPort argument in mana_init_port() Dexuan Cui
2021-10-30 15:30 ` Haiyang Zhang
2021-10-30 20:07 ` Stephen Hemminger
2021-11-01 5:59 ` Dexuan Cui
2021-10-30 0:54 ` [PATCH net-next 2/4] net: mana: Report OS info to the PF driver Dexuan Cui
2021-10-30 15:35 ` Haiyang Zhang
2021-11-01 6:07 ` Dexuan Cui
2021-10-30 0:54 ` [PATCH net-next 3/4] net: mana: Improve the HWC error handling Dexuan Cui
2021-10-30 15:36 ` Haiyang Zhang
2021-10-30 0:54 ` [PATCH net-next 4/4] net: mana: Support hibernation and kexec Dexuan Cui
2021-10-30 15:54 ` Haiyang Zhang
2021-11-01 7:03 ` Dexuan Cui
2021-11-01 15:12 ` Haiyang Zhang [this message]
2021-11-01 16:41 ` Dexuan Cui
2021-11-01 13:30 ` [PATCH net-next 0/4] net: mana: some misc patches patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN8PR21MB128460B5BE376A9E05D43418CA8A9@BN8PR21MB1284.namprd21.prod.outlook.com \
--to=haiyangz@microsoft.com \
--cc=davem@davemloft.net \
--cc=decui@microsoft.com \
--cc=gustavoars@kernel.org \
--cc=kuba@kernel.org \
--cc=kys@microsoft.com \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=olaf@aepfle.de \
--cc=paulros@microsoft.com \
--cc=shacharr@microsoft.com \
--cc=stephen@networkplumber.org \
--cc=vkuznets@redhat.com \
--cc=wei.liu@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).