Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Jun 2023 08:45:38 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Sai Krishna Gajula <saikrishnag@marvell.com>
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
Message-ID: <5e222335-5baa-4ce8-a90b-69a865f29b1a@kadam.mountain>
References: <20230609115806.2625564-1-saikrishnag@marvell.com>
 <880d628e-18bf-44a1-a55f-ffbe1777dd2b@kadam.mountain>
 <BY3PR18MB470788B4096D586DEB9A3B22A023A@BY3PR18MB4707.namprd18.prod.outlook.com>
 <3894dd38-377b-4691-907b-ec3d47fddffd@kadam.mountain>
 <SA1PR18MB4709E390AC13A1EF5F652165A02AA@SA1PR18MB4709.namprd18.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SA1PR18MB4709E390AC13A1EF5F652165A02AA@SA1PR18MB4709.namprd18.prod.outlook.com>
Precedence: bulk

On Fri, Jun 30, 2023 at 05:19:27AM +0000, Sai Krishna Gajula wrote:
> 
> > -----Original Message-----
> > From: Dan Carpenter <dan.carpenter@linaro.org>
> > Sent: Friday, June 23, 2023 5:14 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 23, 2023 at 11:28:19AM +0000, Sai Krishna Gajula wrote:
> > > > 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.
> > >
> > 
> > You're describing how -EPROBE_DEFER is *supposed* to work.  But that's not
> > what this driver does.
> > 
> > If the AF driver is probed before the PTP driver then ptp_probe() should
> > return -EPROBE_DEFER and that would allow the kernel to automatically retry
> > ptp_probe() later.  But instead of that, ptp_probe() returns success.  So I
> > guess the user would have to manually rmmod it and insmod it again?  So,
> > what I'm saying I don't understand why we can't do this in the normal way.
> > 
> > The other thing I'm saying is that the weird return success on error stuff
> > hasn't been tested or we would have discovered the crash through testing.
> > 
> > regards,
> > dan carpenter
> 
> As suggested, we will return error in ptp_probe in case of any
> failure conditions. In this case AF driver continues without PTP support.

I'm concerned about the "AF driver continues without PTP support".

> When the AF driver is probed before PTP driver , we will defer the AF
> probe. Hope these changes are inline with your view.
> I will send a v3 patch with these changes. 
> 

I don't really understand the situation.  You have two drivers.
Normally, the relationship is very simple where you have to load one
before you can load the other.  But here it sounds like the relationships
are very complicated and you are loading one in a degraded state for
some reason...

When drivers are loaded that happens in drivers/base/dd.c.  We start
with a list of drivers to probe.  Then if any of them return
-EPROBE_DEFER, we put them on deferred_probe_pending_list.  Then as soon
as we manage to probe another driver successfully we put the drivers on
deferred_probe_pending_list onto another list and start trying to probe
them again.

That process continues until we've gone through the list of drivers and
nothing succeeds.

regards,
dan carpenter