Thanks for the review On Tue, Jun 17, 2025 at 9:31 PM Simon Horman <horms@xxxxxxxxxx> 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@xxxxxxxxx> > > --- 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.
