Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <20231019212130.3146151-1-dianders@chromium.org>
 <20231019142019.v4.5.Ib2affdbfdc2527aaeef9b46d4f23f7c04147faeb@changeid>
 <eaf05cf1486c418790a1b54cbcda3a98@realtek.com> <CAD=FV=XZQ0XXY7XpX2_ubOwGsi0Hw5otHyuJS2=9QzDJsaSGWg@mail.gmail.com>
In-Reply-To: <CAD=FV=XZQ0XXY7XpX2_ubOwGsi0Hw5otHyuJS2=9QzDJsaSGWg@mail.gmail.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 20 Oct 2023 14:10:42 -0700
Message-ID: <CAD=FV=Vx60LchWqQbALx_tAzK3hnqwSF949KY+R7yWvxfYPQAQ@mail.gmail.com>
Subject: Re: [PATCH v4 5/5] r8152: Block future register access if register
 access fails
To:     Hayes Wang <hayeswang@realtek.com>
Cc:     Jakub Kicinski <kuba@kernel.org>,
        "David S . Miller" <davem@davemloft.net>,
        Grant Grundler <grundler@chromium.org>,
        Edward Hill <ecgh@chromium.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        Simon Horman <horms@kernel.org>,
        Laura Nao <laura.nao@collabora.com>,
        Alan Stern <stern@rowland.harvard.edu>,
        =?UTF-8?Q?Bj=C3=B8rn_Mork?= <bjorn@mork.no>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Fri, Oct 20, 2023 at 8:42=E2=80=AFAM Doug Anderson <dianders@chromium.or=
g> wrote:
>
> > > @@ -8293,6 +8394,8 @@ static int rtl8152_post_reset(struct usb_interf=
ace *intf)
> > >         if (!tp)
> > >                 return 0;
> > >
> > > +       rtl_set_accessible(tp);
> > > +
> >
> > Excuse me. I have a new idea. You could check if it is possible.
> > If you remove test_bit(PROBED_WITH_NO_ERRORS, &tp->flags) in pre_reset(=
),
> > the driver wouldn't be unbound and rebound. Instead, you test PROBED_WI=
TH_NO_ERRORS
> > here to re-initialize the device. Then, you could limit the times of US=
B reset, and
> > the infinite loop wouldn't occur. The code would be like the following,
> >
> >         if (!test_bit(PROBED_WITH_NO_ERRORS, &tp->flags)) {
> >                 /* re-init */
> >                 mutex_lock(&tp->control);
> >                 tp->rtl_ops.init(tp);
> >                 mutex_unlock(&tp->control);
> >                 rtl_hw_phy_work_func_t(&tp->hw_phy_work.work);
> >
> >                 /* re-open(). Maybe move after checking netif_running(n=
etdev) */
> >                 mutex_lock(&tp->control);
> >                 tp->rtl_ops.up(tp);
> >                 mutex_unlock(&tp->control);
> >
> >                 /* check if there is any control error */
> >                 if (test_bit(RTL8152_INACCESSIBLE, &tp->flags) {
> >                         if (tp->reg_access_reset_count < REGISTER_ACCES=
S_MAX_RESETS) {
> >                                 /* queue reset again ? */
> >                         } else {
> >                                 ...
> >                         }
> >                         /* return 0 ? */
> >                 } else {
> >                         set_bit(PROBED_WITH_NO_ERRORS, &tp->flags)
> >                 }
> >         }
>
> The above solution worries me.
>
> I guess one part of this is that it replicates some logic that's in
> probe(). That's not necessarily awful, but we'd at least want to
> reorganize things so that they could share code if possible, though
> maybe that's hard to do with the extra grabs of the mutex?
>
> The other part that worries me is that in the core when we added the
> network device that something in the core might have cached bogus data
> about our network device. This doesn't seem wonderful to me.
>
> I guess yet another part is that your proposed solution there has a
> whole bunch of question marks on it. If it's not necessarily obvious
> what we should do in this case then it doesn't feel like a robust
> solution.
>
> It seems like your main concern here is with the potential for an
> infinite number of resets. I have sent up a small patch to the USB
> core [1] addressing this concern. Let's see what folks say about that
> patch. If it is accepted then it seems like we could just not worry
> about it. If it's not accepted then perhaps feedback on that patch
> will give us additional guidance.
>
> In the meantime I'll at least post v5 since I don't want to leave the
> patch up there with the mismatched mutex. I'll have my v5 point at my
> USB core patch.
>
> [1] https://lore.kernel.org/r/20231020083125.1.I3e5f7abcbf6f08d392e31a582=
6b7f234df662276@changeid

OK, Alan responded to the patch above and suggested simply putting the
retry in the probe routine itself. I think that's actually in the same
spirit as your suggestion but addresses the concerns that I had. I
coded it up and tested it and it seems to work, so I posted that as v5
[2]. Please take a look.

[2] https://lore.kernel.org/r/20231020210751.3415723-1-dianders@chromium.or=
g