netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hayes Wang <hayeswang@realtek.com>
To: "'Doug Anderson'" <dianders@chromium.org>
Cc: "'Jakub Kicinski'" <kuba@kernel.org>,
	"'David S . Miller'" <davem@davemloft.net>,
	"'Alan Stern'" <stern@rowland.harvard.edu>,
	"'Simon Horman'" <horms@kernel.org>,
	"'Edward Hill'" <ecgh@chromium.org>,
	"'Laura Nao'" <laura.nao@collabora.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"'Grant Grundler'" <grundler@chromium.org>,
	"'Bjørn 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>
Subject: RE: [PATCH v3 5/5] r8152: Block future register access if register access fails
Date: Tue, 17 Oct 2023 13:07:17 +0000	[thread overview]
Message-ID: <052401da00fa$dacccd90$906668b0$@realtek.com> (raw)
In-Reply-To: <CAD=FV=U4rGozXHoK8+ejPgRtyoACy1971ftoatQivqzk2tk5ng@mail.gmail.com>

Doug Anderson <dianders@chromium.org>
> Sent: Tuesday, October 17, 2023 12:47 AM
[...
> > >  static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
> > > @@ -8265,6 +8353,19 @@ static int rtl8152_pre_reset(struct
> usb_interface
> > > *intf)
> > >         if (!tp)
> > >                 return 0;
> > >
> > > +       /* We can only use the optimized reset if we made it to the end of
> > > +        * probe without any register access fails, which sets
> > > +        * `PROBED_WITH_NO_ERRORS` to true. If we didn't have that then return
> > > +        * an error here which tells the USB framework to fully unbind/rebind
> > > +        * our driver.
> >
> > Would you stay in a loop of unbind and rebind,
> > if the control transfers in the probe() are not always successful?
> > I just think about the worst case that at least one control always fails in probe().
> 
> We won't! :-) One of the first things that rtl8152_probe() does is to
> call rtl8152_get_version(). That goes through to
> rtl8152_get_version(). That function _doesn't_ queue up a reset if
> there are communication problems, but it does do 3 retries of the
> read. So if all 3 reads fail then we will permanently fail probe,
> which I think is the correct thing to do.

The probe() contains control transfers in
	1. rtl8152_get_version()
	2. tp->rtl_ops.init()

If one of the 3 control transfers in 1) is successful AND
any control transfer in 2) fails,
you would queue a usb reset which would unbind/rebind the driver.
Then, the loop starts.
The loop would be broken, if and only if
	a) all control transfers in 1) fail, OR
	b) all control transfers in 2) succeed.

That is, the loop would be broken when the fail rate of the control transfer is high or low enough.
Otherwise, you would queue a usb reset again and again.
For example, if the fail rate of the control transfer is 10% ~ 60%,
I think you have high probability to keep the loop continually.
Would it never happen?

Best Regards,
Hayes



  reply	other threads:[~2023-10-17 13:08 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-12 19:24 [PATCH v3 0/5] r8152: Avoid writing garbage to the adapter's registers Douglas Anderson
2023-10-12 19:25 ` [PATCH v3 1/5] r8152: Increase USB control msg timeout to 5000ms as per spec Douglas Anderson
2023-10-12 19:25 ` [PATCH v3 2/5] r8152: Check for unplug in rtl_phy_patch_request() Douglas Anderson
2023-10-12 19:25 ` [PATCH v3 3/5] r8152: Check for unplug in r8153b_ups_en() / r8153c_ups_en() Douglas Anderson
2023-10-12 19:25 ` [PATCH v3 4/5] r8152: Rename RTL8152_UNPLUG to RTL8152_INACCESSIBLE Douglas Anderson
2023-10-12 19:25 ` [PATCH v3 5/5] r8152: Block future register access if register access fails Douglas Anderson
2023-10-16  9:15   ` Hayes Wang
2023-10-16 16:46     ` Doug Anderson
2023-10-17 13:07       ` Hayes Wang [this message]
2023-10-17 14:17         ` Doug Anderson
2023-10-17 18:37           ` Doug Anderson
2023-10-18  6:06             ` Grant Grundler
2023-10-18 12:01               ` Hayes Wang
2023-10-18 11:40           ` Hayes Wang
2023-10-19 15:41             ` Doug Anderson

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='052401da00fa$dacccd90$906668b0$@realtek.com' \
    --to=hayeswang@realtek.com \
    --cc=bjorn@mork.no \
    --cc=davem@davemloft.net \
    --cc=dianders@chromium.org \
    --cc=ecgh@chromium.org \
    --cc=edumazet@google.com \
    --cc=grundler@chromium.org \
    --cc=horms@kernel.org \
    --cc=kuba@kernel.org \
    --cc=laura.nao@collabora.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=stern@rowland.harvard.edu \
    /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).