Message-ID: <CAGn2d8MS2PQnosR7AVp=1dRUat_Gu0E9t-P-AQ=k0Ei0ofT_sQ@mail.gmail.com>
Date: Sat, 21 Jun 2025 00:48:57 +0300
From: Abdelrahman Fekry <abdelrahmanfekry375@...il.com>
To: Simon Horman <horms@...nel.org>
Cc: corbet@....net, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, linux-doc@...r.kernel.org, 
	linux-kernel-mentees@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	netdev@...r.kernel.org, skhan@...uxfoundation.com, jacob.e.keller@...el.com, 
	alok.a.tiwari@...cle.com
Subject: Re: [PATCH v2 1/2] docs: net: sysctl documentation cleanup

Thanks for the review


On Tue, Jun 17, 2025 at 9:31 PM Simon Horman <horms@...nel.org> wrote:
>
> On Sun, Jun 15, 2025 at 01:53:23AM +0300, Abdelrahman Fekry wrote:
> > I noticed that some boolean parameters have missing default values
> > (enabled/disabled) in the documentation so i checked the initialization
> > functions to get their default values, also there was some inconsistency
> > in the representation. During the process , i stumbled upon a typo in
> > cipso_rbm_struct_valid instead of cipso_rbm_struct_valid.
>
> Please consider using the imperative mood in patch discriptions.

Noted , will be used in v3.

> As per [*] please denote the target tree for Networking patches.
> In this case net-next seems appropriate.
>
>   [PATCH net-next v3 1/2] ...
>
> [*] https://docs.kernel.org/process/maintainer-netdev.html
>
> And please make sure the patches apply cleanly, without fuzz, on
> top of the target tree: this series seems to apply cleanly neither
> on net or net-next.

Noted, will make sure to denote the target tree and to test it first.

> The text below, up to (but not including your Signed-off-by line)
> doesn't belong in the patch description. If you wish to include
> notes or commentary of this nature then please do so below the
> scissors ("---"). But I think the brief summary you already
> have there is sufficient in this case - we can follow
> the link to v1 for more information.
>
> >
> > Thanks for the review.
> >
> > On Thu, 12 Jun 2025, Jacob Keller wrote:
> > > Would it make sense to use "0 (disabled)" and "1 (enabled)" with
> > > parenthesis for consistency with the default value?
> >
> > Used as suggested.
> >
> > On Fri, 13 Jun 2025, ALOK TIWARI wrote:
> > > for consistency
> > > remove extra space before colon
> > > Default: 1 (enabled)
> >
> > Fixed.
> >
> > On Sat, 14 Jun 2025 10:46:29 -0700, Jakub Kicinski wrote:
> > > You need to repost the entire series. Make sure you read:
> > > https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
> > > before you do.
> >
> > Reposted the entire series, Thanks for you patiency.
> >
> > Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@...il.com>
> > ---

Noted, Thanks.

> > diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> > index 0f1251cce314..68778532faa5 100644
> > --- a/Documentation/networking/ip-sysctl.rst
> > +++ b/Documentation/networking/ip-sysctl.rst
> > @@ -8,14 +8,16 @@ IP Sysctl
> >  ==============================
> >
> >  ip_forward - BOOLEAN
> > -     - 0 - disabled (default)
> > -     - not 0 - enabled
> > +     - 0 (disabled)
> > +     - not 0 (enabled)
> >
> >       Forward Packets between interfaces.
> >
> >       This variable is special, its change resets all configuration
> >       parameters to their default state (RFC1122 for hosts, RFC1812
> >       for routers)
> > +
> > +     Default: 0 (disabled)
> >
> >  ip_default_ttl - INTEGER
> >       Default value of TTL field (Time To Live) for outgoing (but not
> > @@ -75,7 +77,7 @@ fwmark_reflect - BOOLEAN
> >       If unset, these packets have a fwmark of zero. If set, they have the
> >       fwmark of the packet they are replying to.
>
> Maybe it would be more consistent to describe this in terms
> of enabled / disabled rather than set / unset.

Will do this  here and in other parameters to ensure consistency.

>
> >
> > -     Default: 0
> > +     Default: 0 (disabled)
> >
> >  fib_multipath_use_neigh - BOOLEAN
> >       Use status of existing neighbor entry when determining nexthop for
> > @@ -368,7 +370,7 @@ tcp_autocorking - BOOLEAN
> >       queue. Applications can still use TCP_CORK for optimal behavior
> >       when they know how/when to uncork their sockets.
> >
> > -     Default : 1
> > +     Default: 1 (enabled)
>
> For consistency, would it make sense to document the possible values here.

Noted, will document possible values here and in other parameters for
consistency.

>
> >
> >  tcp_available_congestion_control - STRING
> >       Shows the available congestion control choices that are registered.
> > @@ -407,6 +409,12 @@ tcp_congestion_control - STRING
> >
> >  tcp_dsack - BOOLEAN
> >       Allows TCP to send "duplicate" SACKs.
> > +
> > +     Possible values:
> > +             - 0 (disabled)
> > +             - 1 (enabled)
>
> In the case of ip_forward, the possible values are not explicitly named
> as such and appear at the top of the documentation for the parameter.
>
> Here they are explicitly named possible values and appear below the
> description of the parameter, but before documentation of the Default.
> Elsewhere, e.g. ip_forward_use_pmtu, they appear after the documentation of
> the Default. And sometimes, e.g. ip_default_ttl, the possible values are
> documented at all.
>

Noted, will make sure that all representation follow the same appearance,
first the description then possible values then default.

Powered by blists - more mailing lists