On Aug 24 09:33, Corinna Vinschen wrote:
> On Aug 22 13:23, Francois Romieu wrote:
> > Corinna Vinschen <vinsc...@redhat.com> :
> > [...]
> > > That won't happen with the current patch because only
> > > rtl8169_reset_counters would print a log message, it's only called from
> > > open, and open occurs rather seldom. Atop of that the code only tries
> > > to reset counters on HW supporting it, and only if resetting on the HW
> > > fails, there will be a log message at all. There's no reasonable chance
> > > that failing to reset the counters will lead to log flooding.
> >
> > Thanks for reformulating it. We are in violent agreement here.
> >
> > [...]
> > > I'm not trying to avoid work, I'm trying to understand.
> > >
> > > As far as I see it failing to reset the counters has no impact on the
> > > viability of the code. It's still working with offsets and if the
> > > offset is 0 or non-0, the user space won't see the difference in the
> > > values returned by @get_stats64. Successful resetting the counters is
> > > just a bonus.
> >
> > Sorry, my english was really bad:
> >
> > the code should propagate failure when rtl8169_reset_counters and
> > rtl8169_update_counters *simultaneously* fail.
>
> Uhm... sorry, but that still doesn't answer the question. As you can
> see in my patch, the initalization at open time is already encapsulated
> in a function rtl8169_init_counter_offsets.
>
> Assuming rtl8169_init_counter_offsets returns -1 if both functions,
> rtl8169_reset_counters and rtl8169_update_counters fail.
>
> Then... what?
>
> Not being able to reset or update the counters is still not at all fatal
> for the operation of the NIC as a whole and rtl_open in particular:
>
> rtl_open()
> {
> [...]
>
> /* This is non-fatal. */
> if (!rtl8169_init_counter_offsets ()) {
>
> /* What to do here??? */
>
> }
>
> [...]
> }Never mind, I rearranged the code so that the warning is now printed in rtl_open. All functions related to the counter reset/update are now bool functions. I just sent a v3 patch. Corinna
pgp7HuqR5UDnR.pgp
Description: PGP signature
