From: Sai Krishna Gajula <saikrishnag@marvell.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
"edumazet@google.com" <edumazet@google.com>,
"kuba@kernel.org" <kuba@kernel.org>,
"pabeni@redhat.com" <pabeni@redhat.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Sunil Kovvuri Goutham <sgoutham@marvell.com>,
"maciej.fijalkowski@intel.com" <maciej.fijalkowski@intel.com>,
Naveen Mamindlapalli <naveenm@marvell.com>
Subject: Re: [net PATCH v2] octeontx2-af: Move validation of ptp pointer before its usage
Date: Fri, 23 Jun 2023 11:28:19 +0000 [thread overview]
Message-ID: <BY3PR18MB470788B4096D586DEB9A3B22A023A@BY3PR18MB4707.namprd18.prod.outlook.com> (raw)
In-Reply-To: <880d628e-18bf-44a1-a55f-ffbe1777dd2b@kadam.mountain>
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Sent: Friday, June 9, 2023 6:48 PM
> To: Sai Krishna Gajula <saikrishnag@marvell.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> Sunil Kovvuri Goutham <sgoutham@marvell.com>;
> maciej.fijalkowski@intel.com; Naveen Mamindlapalli
> <naveenm@marvell.com>
> Subject: Re: [net PATCH v2] octeontx2-af: Move validation of ptp
> pointer before its usage
>
> On Fri, Jun 09, 2023 at 05:28:06PM +0530, Sai Krishna wrote:
> > @@ -428,7 +427,7 @@ static int ptp_probe(struct pci_dev *pdev,
> > return 0;
> >
> > error_free:
> > - devm_kfree(dev, ptp);
> > + kfree(ptp);
>
> Yeah. It's strange any time we call devm_kfree()... So there is something
> here which I have not understood.
We moved from devm_kzalloc/devm_kfree to kzalloc/kfree as per review comments from Maciej.
>
> >
> > error:
> > /* For `ptp_get()` we need to differentiate between the case
>
> This probe function is super weird how it returns success on the failure path.
> One concern, I had initially was that if anything returns -EPROBE_DEFER then
> we cannot recover. That's not possible in the current code, but it makes me
> itch... But here is a different crash.
>
In few circumstances, the PTP device is probed before the AF device in the driver. In such instance, -EPROBE_DEFER is used.
-- EDEFER_PROBE is useful when probe order changes. Ex: AF driver probes before PTP.
> drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> 432 error:
> 433 /* For `ptp_get()` we need to differentiate between the case
> 434 * when the core has not tried to probe this device and the case
> when
> 435 * the probe failed. In the later case we pretend that the
> 436 * initialization was successful and keep the error in
> 437 * `dev->driver_data`.
> 438 */
> 439 pci_set_drvdata(pdev, ERR_PTR(err));
> 440 if (!first_ptp_block)
> 441 first_ptp_block = ERR_PTR(err);
>
> first_ptp_block is NULL for unprobed, an error pointer for probe failure, or
> valid pointer.
This is correct.
>
> 442
> 443 return 0;
> 444 }
>
> drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> 201 struct ptp *ptp_get(void)
> 202 {
> 203 struct ptp *ptp = first_ptp_block;
> ^^^^^^^^^^^^^^^^^^^^^^
>
> 204
> 205 /* Check PTP block is present in hardware */
> 206 if (!pci_dev_present(ptp_id_table))
> 207 return ERR_PTR(-ENODEV);
> 208 /* Check driver is bound to PTP block */
> 209 if (!ptp)
> 210 ptp = ERR_PTR(-EPROBE_DEFER);
> 211 else
> 212 pci_dev_get(ptp->pdev);
> ^^^^^^^^^ if first_ptp_block is an error pointer this will
> Oops.
Will fix this in v3 patch.
Thanks,
Sai
>
> 213
> 214 return ptp;
> 215 }
>
> regards,
> dan carpenter
next prev parent reply other threads:[~2023-06-23 11:28 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-09 11:58 [net PATCH v2] octeontx2-af: Move validation of ptp pointer before its usage Sai Krishna
2023-06-09 13:18 ` Dan Carpenter
2023-06-23 11:28 ` Sai Krishna Gajula [this message]
2023-06-23 11:44 ` Dan Carpenter
2023-06-30 5:19 ` Sai Krishna Gajula
2023-06-30 5:45 ` Dan Carpenter
2023-06-30 6:46 ` Sunil Kovvuri
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=BY3PR18MB470788B4096D586DEB9A3B22A023A@BY3PR18MB4707.namprd18.prod.outlook.com \
--to=saikrishnag@marvell.com \
--cc=dan.carpenter@linaro.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maciej.fijalkowski@intel.com \
--cc=naveenm@marvell.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=sgoutham@marvell.com \
/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).