Linux Documentation
 help / color / mirror / Atom feed
* Re: [PATCH v2 2/2] Docs: hwmon: pmbus: Add PXE1610 driver
From: Vijay Khemka @ 2019-06-06 18:49 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Jean Delvare, Jonathan Corbet, linux-hwmon@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari,
	Greg Kroah-Hartman
In-Reply-To: <20190605204811.GA32379@roeck-us.net>



On 6/5/19, 1:48 PM, "Guenter Roeck" <groeck7@gmail.com on behalf of linux@roeck-us.net> wrote:

    On Thu, May 30, 2019 at 04:11:57PM -0700, Vijay Khemka wrote:
    > Added support for Infenion PXE1610 driver
    > 
    Applied, after fixing
    	s/Infenion/Infineon/
    	s/Infinion/Infineon/
Thanks
    
    Guenter
    
    > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
    > ---
    > Changes in v2:
    > incorporated all the feedback from Guenter Roeck <linux@roeck-us.net>
    > 
    >  Documentation/hwmon/pxe1610 | 90 +++++++++++++++++++++++++++++++++++++
    >  1 file changed, 90 insertions(+)
    >  create mode 100644 Documentation/hwmon/pxe1610
    > 
    > diff --git a/Documentation/hwmon/pxe1610 b/Documentation/hwmon/pxe1610
    > new file mode 100644
    > index 000000000000..24825db8736f
    > --- /dev/null
    > +++ b/Documentation/hwmon/pxe1610
    > @@ -0,0 +1,90 @@
    > +Kernel driver pxe1610
    > +=====================
    > +
    > +Supported chips:
    > +  * Infinion PXE1610
    > +    Prefix: 'pxe1610'
    > +    Addresses scanned: -
    > +    Datasheet: Datasheet is not publicly available.
    > +
    > +  * Infinion PXE1110
    > +    Prefix: 'pxe1110'
    > +    Addresses scanned: -
    > +    Datasheet: Datasheet is not publicly available.
    > +
    > +  * Infinion PXM1310
    > +    Prefix: 'pxm1310'
    > +    Addresses scanned: -
    > +    Datasheet: Datasheet is not publicly available.
    > +
    > +Author: Vijay Khemka <vijaykhemka@fb.com>
    > +
    > +
    > +Description
    > +-----------
    > +
    > +PXE1610/PXE1110 are Multi-rail/Multiphase Digital Controllers
    > +and compliant to
    > +	-- Intel VR13 DC-DC converter specifications.
    > +	-- Intel SVID protocol.
    > +Used for Vcore power regulation for Intel VR13 based microprocessors
    > +	-- Servers, Workstations, and High-end desktops
    > +
    > +PXM1310 is a Multi-rail Controllers and it is compliant to
    > +	-- Intel VR13 DC-DC converter specifications.
    > +	-- Intel SVID protocol.
    > +Used for DDR3/DDR4 Memory power regulation for Intel VR13 and
    > +IMVP8 based systems
    > +
    > +
    > +Usage Notes
    > +-----------
    > +
    > +This driver does not probe for PMBus devices. You will have
    > +to instantiate devices explicitly.
    > +
    > +Example: the following commands will load the driver for an PXE1610
    > +at address 0x70 on I2C bus #4:
    > +
    > +# modprobe pxe1610
    > +# echo pxe1610 0x70 > /sys/bus/i2c/devices/i2c-4/new_device
    > +
    > +It can also be instantiated by declaring in device tree
    > +
    > +
    > +Sysfs attributes
    > +----------------
    > +
    > +curr1_label		"iin"
    > +curr1_input		Measured input current
    > +curr1_alarm		Current high alarm
    > +
    > +curr[2-4]_label		"iout[1-3]"
    > +curr[2-4]_input		Measured output current
    > +curr[2-4]_crit		Critical maximum current
    > +curr[2-4]_crit_alarm	Current critical high alarm
    > +
    > +in1_label		"vin"
    > +in1_input		Measured input voltage
    > +in1_crit		Critical maximum input voltage
    > +in1_crit_alarm		Input voltage critical high alarm
    > +
    > +in[2-4]_label		"vout[1-3]"
    > +in[2-4]_input		Measured output voltage
    > +in[2-4]_lcrit		Critical minimum output voltage
    > +in[2-4]_lcrit_alarm	Output voltage critical low alarm
    > +in[2-4]_crit		Critical maximum output voltage
    > +in[2-4]_crit_alarm	Output voltage critical high alarm
    > +
    > +power1_label		"pin"
    > +power1_input		Measured input power
    > +power1_alarm		Input power high alarm
    > +
    > +power[2-4]_label	"pout[1-3]"
    > +power[2-4]_input	Measured output power
    > +
    > +temp[1-3]_input		Measured temperature
    > +temp[1-3]_crit		Critical high temperature
    > +temp[1-3]_crit_alarm	Chip temperature critical high alarm
    > +temp[1-3]_max		Maximum temperature
    > +temp[1-3]_max_alarm	Chip temperature high alarm
    


^ permalink raw reply

* Re: [PATCH v1] docs/core-api: Add string helpers API to the list
From: Jonathan Corbet @ 2019-06-06 16:01 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-doc, Andrew Morton, Mike Rapoport
In-Reply-To: <20190605163944.50803-1-andriy.shevchenko@linux.intel.com>

On Wed,  5 Jun 2019 19:39:44 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Some times string helpers are needed, but there is nothing about them
> in the generated documentation.
> 
> Fill the gap by adding a reference to string_helpers.c exported functions.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

So I've applied this (and the integer power functions one too), but let me
grumble for just a moment...

This patch adds a set of new warnings to the docs build:

> +./lib/string_helpers.c:236: WARNING: Unexpected indentation.
> +./lib/string_helpers.c:241: WARNING: Block quote ends without a blank line; unexpected unindent.
> +./lib/string_helpers.c:446: WARNING: Unexpected indentation.
> +./lib/string_helpers.c:451: WARNING: Block quote ends without a blank line; unexpected unindent.
> +./lib/string_helpers.c:474: WARNING: Unexpected indentation.

It would be *really* nice if folks would check for these things and fix
them when they arise.  The docs build is a horrific mess of warnings that
will never get better if we keep adding more.  This one is an easy fix;
I'll toss together a patch to do it.

Beyond that (and this is in no way your fault, I'm just whining)
kernel-api.rst has become a huge dumping ground.  Someday it would sure be
nice if we could create a bit more order there...

Thanks,

jon

^ permalink raw reply

* Re: [PATCH] docs: filesystems: vfs: Render method descriptions
From: Jonathan Corbet @ 2019-06-06 15:46 UTC (permalink / raw)
  To: Tobin C. Harding
  Cc: Al Viro, Neil Brown, Randy Dunlap, linux-doc, linux-fsdevel,
	linux-kernel
In-Reply-To: <20190604002656.30925-1-tobin@kernel.org>

On Tue,  4 Jun 2019 10:26:56 +1000
"Tobin C. Harding" <tobin@kernel.org> wrote:

> Currently vfs.rst does not render well into HTML the method descriptions
> for VFS data structures.  We can improve the HTML output by putting the
> description string on a new line following the method name.
> 
> Suggested-by: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
> 
> Jon,
> 
> As discussed on LKML; this patch applies on top of the series
> 
> 	[PATCH v4 0/9] docs: Convert VFS doc to RST
> 
> If it does not apply cleanly to your branch please feel free to ask me
> to fix it.

There was one merge conflict, but nothing too serious.  I've applied it,
and things look a lot better - thanks!

jon

^ permalink raw reply

* Re: [PATCH v3 0/2] ima/evm fixes for v5.2
From: Roberto Sassu @ 2019-06-06 15:22 UTC (permalink / raw)
  To: Mimi Zohar, dmitry.kasatkin, mjg59
  Cc: linux-integrity, linux-security-module, linux-doc, stable,
	linux-kernel, silviu.vlasceanu
In-Reply-To: <1559832596.4278.124.camel@linux.ibm.com>

On 6/6/2019 4:49 PM, Mimi Zohar wrote:
> On Thu, 2019-06-06 at 13:43 +0200, Roberto Sassu wrote:
>> On 6/6/2019 1:26 PM, Roberto Sassu wrote:
>>> Previous versions included the patch 'ima: don't ignore INTEGRITY_UNKNOWN
>>> EVM status'. However, I realized that this patch cannot be accepted alone
>>> because IMA-Appraisal would deny access to new files created during the
>>> boot. With the current behavior, those files are accessible because they
>>> have a valid security.ima (not protected by EVM) created after the first
>>> write.
>>>
>>> A solution for this problem is to initialize EVM very early with a random
>>> key. Access to created files will be granted, even with the strict
>>> appraisal, because after the first write those files will have both
>>> security.ima and security.evm (HMAC calculated with the random key).
>>>
>>> Strict appraisal will work only if it is done with signatures until the
>>> persistent HMAC key is loaded.
>>
>> Changelog
>>
>> v2:
>> - remove patch 1/3 (evm: check hash algorithm passed to init_desc());
>>     already accepted
>> - remove patch 3/3 (ima: show rules with IMA_INMASK correctly);
>>     already accepted
>> - add new patch (evm: add option to set a random HMAC key at early boot)
>> - patch 2/3: modify patch description
> 
> Roberto, as I tried explaining previously, this feature is not a
> simple bug fix.  These patches, if upstreamed, will be upstreamed the
> normal way, during an open window.  Whether they are classified as a
> bug fix has yet to be decided.

Sorry, I understood that I can claim that there is a bug. I provided a
motivation in patch 2/2.


> Please stop Cc'ing stable.  If I don't Cc stable before sending the pull request, then Greg and Sasha have been really good about deciding which patches should be backported.  (Please refer to the comment on "Cc'ing stable" in section "5) Select the recipients for your patch" in Documentation/process/submitting-patches.rst.)
> 
> I'll review these patches, but in the future please use an appropriate patch set cover letter title in the subject line.

Ok.

Thanks

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply

* Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC
From: Ezequiel Garcia @ 2019-06-06 15:12 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Guenter Roeck, Enric Balletbo i Serra, linux-kernel,
	Gwendal Grignou, Guenter Roeck, Benson Leung, Lee Jones, kernel,
	Dmitry Torokhov, Gustavo Pimentel, Randy Dunlap,
	Lorenzo Pieralisi, linux-doc, Enno Luebbers, Guido Kiener,
	Thomas Gleixner, Kishon Vijay Abraham I, Jonathan Corbet, Wu Hao,
	Kate Stewart, Tycho Andersen, Gerd Hoffmann, Jilayne Lovejoy
In-Reply-To: <20190606145121.GA13048@kroah.com>

On Thu, 2019-06-06 at 16:51 +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 06, 2019 at 11:01:17AM -0300, Ezequiel Garcia wrote:
> > On Tue, 2019-06-04 at 20:59 +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > > > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > > > Hey Greg,
> > > > > > 
> > > > > > > > + dev_info(&pdev->dev, "Created misc device /dev/%s\n",
> > > > > > > > +          data->misc.name);
> > > > > > > 
> > > > > > > No need to be noisy, if all goes well, your code should be quiet.
> > > > > > > 
> > > > > > 
> > > > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > > > hijack this thread for this discussion.
> > > > > > 
> > > > > > > From a kernel developer point-of-view, or even from a platform
> > > > > > developer or user with a debugging hat point-of-view, having
> > > > > > a "device created" or "device registered" message is often very useful.
> > > > > 
> > > > > For you, yes.  For someone with 30000 devices attached to their system,
> > > > > it is not, and causes booting to take longer than it should be.
> > > > > 
> > > > > > In fact, I wish people would do this more often, so I don't have to
> > > > > > deal with dynamic debug, or hack my way:
> > > > > > 
> > > > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > > > index 4589631798c9..473549b26bb2 100644
> > > > > > --- a/drivers/media/i2c/ov5647.c
> > > > > > +++ b/drivers/media/i2c/ov5647.c
> > > > > > @@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
> > > > > >         if (ret < 0)
> > > > > >                 goto error;
> > > > > > 
> > > > > > -       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
> > > > > > +       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
> > > > > >         return 0;
> > > > > >  error:
> > > > > >         media_entity_cleanup(&sd->entity);
> > > > > > 
> > > > > > In some subsystems, it's even a behavior I'm more or less relying on:
> > > > > > 
> > > > > > $ git grep v4l2_info.*registered drivers/media/ | wc -l
> > > > > > 26
> > > > > > 
> > > > > > And on the downsides, I can't find much. It's just one little line,
> > > > > > that is not even noticed unless you have logging turned on.
> > > > > 
> > > > > Its better to be quiet, which is why the "default driver registration"
> > > > > macros do not have any printk messages in them.  When converting drivers
> > > > > over to it, we made the boot process much more sane, don't try to go and
> > > > > add messages for no good reason back in please.
> > > > > 
> > > > > dynamic debugging can be enabled on a module and line-by-line basis,
> > > > > even from the boot command line.  So if you need debugging, you can
> > > > > always ask someone to just reboot or unload/load the module and get the
> > > > > message that way.
> > > > > 
> > > > 
> > > > Can we by any chance make this an official policy ? I am kind of tired
> > > > having to argue about this over and over again.
> > > 
> > > Sure, but how does anyone make any "official policy" in the kernel?  :)
> > > 
> > > I could just go through and delete all "look ma, a new driver/device!"
> > > messages, but that might be annoying...
> > > 
> > 
> > Well, I really need to task.
> 
> ???
> 

Oops, typo: s/task/ask :-)

> > If it's not an official policy (and won't be anytime soon?),
> 
> The ":)" there was that we really have very few "official" policies,
> only things that we all strongly encourage to happen.  And get grumpy if
> we see them in code reviews.  Like I did here.
> 

Well, not everyone gets grumpy. As I pointed out, we use this "registered"
messages (messages or noise, seems this lie in the eye of the beholder),
consistently across entire subsystems.

> > then what's preventing Enric from pushing this print on this driver,
> > given he is the one maintaining the code?
> 
> Given that he wants people to review his code, why would you tell him to
> ignore what people are trying to tell him?
> 

I'm not suggesting to ignore anyone, rather to consider all voices
involved in each review comment.

> Again, don't be noisy, it's not hard, and is how things have been
> trending for many years now.
> 

Thanks,
Eze


^ permalink raw reply

* Re: [char-misc-next 3/7 RESEND] mei: docs: update mei documentation
From: Greg Kroah-Hartman @ 2019-06-06 14:51 UTC (permalink / raw)
  To: Tomas Winkler; +Cc: Alexander Usyskin, linux-kernel, Jonathan Corbet, linux-doc
In-Reply-To: <20190606133108.26964-1-tomas.winkler@intel.com>

On Thu, Jun 06, 2019 at 04:31:08PM +0300, Tomas Winkler wrote:
> The mei driver went via multiple changes, update
> the documentation and fix formatting.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  Documentation/driver-api/mei/mei.rst | 96 ++++++++++++++++++----------
>  1 file changed, 61 insertions(+), 35 deletions(-)

that worked, thanks!

greg k-h

^ permalink raw reply

* Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC
From: Greg Kroah-Hartman @ 2019-06-06 14:51 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Guenter Roeck, Enric Balletbo i Serra, linux-kernel,
	Gwendal Grignou, Guenter Roeck, Benson Leung, Lee Jones, kernel,
	Dmitry Torokhov, Gustavo Pimentel, Randy Dunlap,
	Lorenzo Pieralisi, linux-doc, Enno Luebbers, Guido Kiener,
	Thomas Gleixner, Kishon Vijay Abraham I, Jonathan Corbet, Wu Hao,
	Kate Stewart, Tycho Andersen, Gerd Hoffmann, Jilayne Lovejoy
In-Reply-To: <bda48bf80add26153e531912fbfca25071934c94.camel@collabora.com>

On Thu, Jun 06, 2019 at 11:01:17AM -0300, Ezequiel Garcia wrote:
> On Tue, 2019-06-04 at 20:59 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > > Hey Greg,
> > > > > 
> > > > > > > + dev_info(&pdev->dev, "Created misc device /dev/%s\n",
> > > > > > > +          data->misc.name);
> > > > > > 
> > > > > > No need to be noisy, if all goes well, your code should be quiet.
> > > > > > 
> > > > > 
> > > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > > hijack this thread for this discussion.
> > > > > 
> > > > > > From a kernel developer point-of-view, or even from a platform
> > > > > developer or user with a debugging hat point-of-view, having
> > > > > a "device created" or "device registered" message is often very useful.
> > > > 
> > > > For you, yes.  For someone with 30000 devices attached to their system,
> > > > it is not, and causes booting to take longer than it should be.
> > > > 
> > > > > In fact, I wish people would do this more often, so I don't have to
> > > > > deal with dynamic debug, or hack my way:
> > > > > 
> > > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > > index 4589631798c9..473549b26bb2 100644
> > > > > --- a/drivers/media/i2c/ov5647.c
> > > > > +++ b/drivers/media/i2c/ov5647.c
> > > > > @@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
> > > > >         if (ret < 0)
> > > > >                 goto error;
> > > > > 
> > > > > -       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
> > > > > +       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
> > > > >         return 0;
> > > > >  error:
> > > > >         media_entity_cleanup(&sd->entity);
> > > > > 
> > > > > In some subsystems, it's even a behavior I'm more or less relying on:
> > > > > 
> > > > > $ git grep v4l2_info.*registered drivers/media/ | wc -l
> > > > > 26
> > > > > 
> > > > > And on the downsides, I can't find much. It's just one little line,
> > > > > that is not even noticed unless you have logging turned on.
> > > > 
> > > > Its better to be quiet, which is why the "default driver registration"
> > > > macros do not have any printk messages in them.  When converting drivers
> > > > over to it, we made the boot process much more sane, don't try to go and
> > > > add messages for no good reason back in please.
> > > > 
> > > > dynamic debugging can be enabled on a module and line-by-line basis,
> > > > even from the boot command line.  So if you need debugging, you can
> > > > always ask someone to just reboot or unload/load the module and get the
> > > > message that way.
> > > > 
> > > 
> > > Can we by any chance make this an official policy ? I am kind of tired
> > > having to argue about this over and over again.
> > 
> > Sure, but how does anyone make any "official policy" in the kernel?  :)
> > 
> > I could just go through and delete all "look ma, a new driver/device!"
> > messages, but that might be annoying...
> > 
> 
> Well, I really need to task.

???

> If it's not an official policy (and won't be anytime soon?),

The ":)" there was that we really have very few "official" policies,
only things that we all strongly encourage to happen.  And get grumpy if
we see them in code reviews.  Like I did here.

> then what's preventing Enric from pushing this print on this driver,
> given he is the one maintaining the code?

Given that he wants people to review his code, why would you tell him to
ignore what people are trying to tell him?

Again, don't be noisy, it's not hard, and is how things have been
trending for many years now.

thanks,

greg k-h

^ permalink raw reply

* Re: [PATCH v3 0/2] ima/evm fixes for v5.2
From: Mimi Zohar @ 2019-06-06 14:49 UTC (permalink / raw)
  To: Roberto Sassu, dmitry.kasatkin, mjg59
  Cc: linux-integrity, linux-security-module, linux-doc, stable,
	linux-kernel, silviu.vlasceanu
In-Reply-To: <3711f387-3aef-9fbb-1bb4-dded6807b033@huawei.com>

On Thu, 2019-06-06 at 13:43 +0200, Roberto Sassu wrote:
> On 6/6/2019 1:26 PM, Roberto Sassu wrote:
> > Previous versions included the patch 'ima: don't ignore INTEGRITY_UNKNOWN
> > EVM status'. However, I realized that this patch cannot be accepted alone
> > because IMA-Appraisal would deny access to new files created during the
> > boot. With the current behavior, those files are accessible because they
> > have a valid security.ima (not protected by EVM) created after the first
> > write.
> > 
> > A solution for this problem is to initialize EVM very early with a random
> > key. Access to created files will be granted, even with the strict
> > appraisal, because after the first write those files will have both
> > security.ima and security.evm (HMAC calculated with the random key).
> > 
> > Strict appraisal will work only if it is done with signatures until the
> > persistent HMAC key is loaded.
> 
> Changelog
> 
> v2:
> - remove patch 1/3 (evm: check hash algorithm passed to init_desc());
>    already accepted
> - remove patch 3/3 (ima: show rules with IMA_INMASK correctly);
>    already accepted
> - add new patch (evm: add option to set a random HMAC key at early boot)
> - patch 2/3: modify patch description

Roberto, as I tried explaining previously, this feature is not a
simple bug fix.  These patches, if upstreamed, will be upstreamed the
normal way, during an open window.  Whether they are classified as a
bug fix has yet to be decided.

Please stop Cc'ing stable.  If I don't Cc stable before sending the pull request, then Greg and Sasha have been really good about deciding which patches should be backported.  (Please refer to the comment on "Cc'ing stable" in section "5) Select the recipients for your patch" in Documentation/process/submitting-patches.rst.)

I'll review these patches, but in the future please use an appropriate patch set cover letter title in the subject line.

thanks,

Mimi


> 
> v1:
> - remove patch 2/4 (evm: reset status in evm_inode_post_setattr()); file
>    attributes cannot be set if the signature is portable and immutable
> - patch 3/4: add __ro_after_init to ima_appraise_req_evm variable
>    declaration
> - patch 3/4: remove ima_appraise_req_evm kernel option and introduce
>    'enforce-evm' and 'log-evm' as possible values for ima_appraise=
> - remove patch 4/4 (ima: only audit failed appraisal verifications)
> - add new patch (ima: show rules with IMA_INMASK correctly)
> 
> 
> > Roberto Sassu (2):
> >    evm: add option to set a random HMAC key at early boot
> >    ima: add enforce-evm and log-evm modes to strictly check EVM status
> > 
> >   .../admin-guide/kernel-parameters.txt         | 11 ++--
> >   security/integrity/evm/evm.h                  | 10 +++-
> >   security/integrity/evm/evm_crypto.c           | 57 ++++++++++++++++---
> >   security/integrity/evm/evm_main.c             | 41 ++++++++++---
> >   security/integrity/ima/ima_appraise.c         |  8 +++
> >   security/integrity/integrity.h                |  1 +
> >   6 files changed, 106 insertions(+), 22 deletions(-)
> > 
> 


^ permalink raw reply

* Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC
From: Ezequiel Garcia @ 2019-06-06 14:01 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Guenter Roeck
  Cc: Enric Balletbo i Serra, linux-kernel, Gwendal Grignou,
	Guenter Roeck, Benson Leung, Lee Jones, kernel, Dmitry Torokhov,
	Gustavo Pimentel, Randy Dunlap, Lorenzo Pieralisi, linux-doc,
	Enno Luebbers, Guido Kiener, Thomas Gleixner,
	Kishon Vijay Abraham I, Jonathan Corbet, Wu Hao, Kate Stewart,
	Tycho Andersen, Gerd Hoffmann, Jilayne Lovejoy
In-Reply-To: <20190604185953.GA2061@kroah.com>

On Tue, 2019-06-04 at 20:59 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > Hey Greg,
> > > > 
> > > > > > + dev_info(&pdev->dev, "Created misc device /dev/%s\n",
> > > > > > +          data->misc.name);
> > > > > 
> > > > > No need to be noisy, if all goes well, your code should be quiet.
> > > > > 
> > > > 
> > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > hijack this thread for this discussion.
> > > > 
> > > > > From a kernel developer point-of-view, or even from a platform
> > > > developer or user with a debugging hat point-of-view, having
> > > > a "device created" or "device registered" message is often very useful.
> > > 
> > > For you, yes.  For someone with 30000 devices attached to their system,
> > > it is not, and causes booting to take longer than it should be.
> > > 
> > > > In fact, I wish people would do this more often, so I don't have to
> > > > deal with dynamic debug, or hack my way:
> > > > 
> > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > index 4589631798c9..473549b26bb2 100644
> > > > --- a/drivers/media/i2c/ov5647.c
> > > > +++ b/drivers/media/i2c/ov5647.c
> > > > @@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
> > > >         if (ret < 0)
> > > >                 goto error;
> > > > 
> > > > -       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
> > > > +       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
> > > >         return 0;
> > > >  error:
> > > >         media_entity_cleanup(&sd->entity);
> > > > 
> > > > In some subsystems, it's even a behavior I'm more or less relying on:
> > > > 
> > > > $ git grep v4l2_info.*registered drivers/media/ | wc -l
> > > > 26
> > > > 
> > > > And on the downsides, I can't find much. It's just one little line,
> > > > that is not even noticed unless you have logging turned on.
> > > 
> > > Its better to be quiet, which is why the "default driver registration"
> > > macros do not have any printk messages in them.  When converting drivers
> > > over to it, we made the boot process much more sane, don't try to go and
> > > add messages for no good reason back in please.
> > > 
> > > dynamic debugging can be enabled on a module and line-by-line basis,
> > > even from the boot command line.  So if you need debugging, you can
> > > always ask someone to just reboot or unload/load the module and get the
> > > message that way.
> > > 
> > 
> > Can we by any chance make this an official policy ? I am kind of tired
> > having to argue about this over and over again.
> 
> Sure, but how does anyone make any "official policy" in the kernel?  :)
> 
> I could just go through and delete all "look ma, a new driver/device!"
> messages, but that might be annoying...
> 

Well, I really need to task.

If it's not an official policy (and won't be anytime soon?), then
what's preventing Enric from pushing this print on this driver,
given he is the one maintaining the code?

Thanks,
Eze


^ permalink raw reply

* [char-misc-next 3/7 RESEND] mei: docs: update mei documentation
From: Tomas Winkler @ 2019-06-06 13:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Alexander Usyskin, linux-kernel, Tomas Winkler, Jonathan Corbet,
	linux-doc

The mei driver went via multiple changes, update
the documentation and fix formatting.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 Documentation/driver-api/mei/mei.rst | 96 ++++++++++++++++++----------
 1 file changed, 61 insertions(+), 35 deletions(-)

diff --git a/Documentation/driver-api/mei/mei.rst b/Documentation/driver-api/mei/mei.rst
index c7f10a4b46ff..c800d8e5f422 100644
--- a/Documentation/driver-api/mei/mei.rst
+++ b/Documentation/driver-api/mei/mei.rst
@@ -5,34 +5,32 @@ Introduction
 
 The Intel Management Engine (Intel ME) is an isolated and protected computing
 resource (Co-processor) residing inside certain Intel chipsets. The Intel ME
-provides support for computer/IT management features. The feature set
-depends on the Intel chipset SKU.
+provides support for computer/IT management and security features.
+The actual feature set depends on the Intel chipset SKU.
 
 The Intel Management Engine Interface (Intel MEI, previously known as HECI)
 is the interface between the Host and Intel ME. This interface is exposed
-to the host as a PCI device. The Intel MEI Driver is in charge of the
-communication channel between a host application and the Intel ME feature.
+to the host as a PCI device, actually multiple PCI devices might be exposed.
+The Intel MEI Driver is in charge of the communication channel between
+a host application and the Intel ME features.
 
-Each Intel ME feature (Intel ME Client) is addressed by a GUID/UUID and
+Each Intel ME feature, or Intel ME Client is addressed by a unique GUID and
 each client has its own protocol. The protocol is message-based with a
-header and payload up to 512 bytes.
+header and payload up to maximal number of bytes advertised by the client,
+upon connection.
 
 Intel MEI Driver
 ================
 
-The driver exposes a misc device called /dev/mei.
+The driver exposes a character device with device nodes /dev/meiX.
 
 An application maintains communication with an Intel ME feature while
-/dev/mei is open. The binding to a specific feature is performed by calling
-MEI_CONNECT_CLIENT_IOCTL, which passes the desired UUID.
+/dev/meiX is open. The binding to a specific feature is performed by calling
+:c:macro:`MEI_CONNECT_CLIENT_IOCTL`, which passes the desired GUID.
 The number of instances of an Intel ME feature that can be opened
 at the same time depends on the Intel ME feature, but most of the
 features allow only a single instance.
 
-The Intel AMT Host Interface (Intel AMTHI) feature supports multiple
-simultaneous user connected applications. The Intel MEI driver
-handles this internally by maintaining request queues for the applications.
-
 The driver is transparent to data that are passed between firmware feature
 and host application.
 
@@ -40,6 +38,8 @@ Because some of the Intel ME features can change the system
 configuration, the driver by default allows only a privileged
 user to access it.
 
+The session is terminated calling :c:func:`close(int fd)`.
+
 A code snippet for an application communicating with Intel AMTHI client:
 
 .. code-block:: C
@@ -47,13 +47,13 @@ A code snippet for an application communicating with Intel AMTHI client:
 	struct mei_connect_client_data data;
 	fd = open(MEI_DEVICE);
 
-	data.d.in_client_uuid = AMTHI_UUID;
+	data.d.in_client_uuid = AMTHI_GUID;
 
 	ioctl(fd, IOCTL_MEI_CONNECT_CLIENT, &data);
 
 	printf("Ver=%d, MaxLen=%ld\n",
-			data.d.in_client_uuid.protocol_version,
-			data.d.in_client_uuid.max_msg_length);
+	       data.d.in_client_uuid.protocol_version,
+	       data.d.in_client_uuid.max_msg_length);
 
 	[...]
 
@@ -67,60 +67,86 @@ A code snippet for an application communicating with Intel AMTHI client:
 	close(fd);
 
 
-IOCTLs
-======
+User space API
+
+IOCTLs:
+=======
 
 The Intel MEI Driver supports the following IOCTL commands:
-	IOCTL_MEI_CONNECT_CLIENT	Connect to firmware Feature (client).
 
-	usage:
-		struct mei_connect_client_data clientData;
-		ioctl(fd, IOCTL_MEI_CONNECT_CLIENT, &clientData);
+IOCTL_MEI_CONNECT_CLIENT
+-------------------------
+Connect to firmware Feature/Client.
+
+.. code-block:: none
+
+	Usage:
 
-	inputs:
-		mei_connect_client_data struct contain the following
-		input field:
+        struct mei_connect_client_data client_data;
 
-		in_client_uuid -	UUID of the FW Feature that needs
+        ioctl(fd, IOCTL_MEI_CONNECT_CLIENT, &client_data);
+
+	Inputs:
+
+        struct mei_connect_client_data - contain the following
+	Input field:
+
+		in_client_uuid -	GUID of the FW Feature that needs
 					to connect to.
-	outputs:
+         Outputs:
 		out_client_properties - Client Properties: MTU and Protocol Version.
 
-	error returns:
+         Error returns:
+
+                ENOTTY  No such client (i.e. wrong GUID) or connection is not allowed.
 		EINVAL	Wrong IOCTL Number
-		ENODEV	Device or Connection is not initialized or ready. (e.g. Wrong UUID)
+		ENODEV	Device or Connection is not initialized or ready.
 		ENOMEM	Unable to allocate memory to client internal data.
 		EFAULT	Fatal Error (e.g. Unable to access user input data)
 		EBUSY	Connection Already Open
 
-	Notes:
+:Note:
         max_msg_length (MTU) in client properties describes the maximum
         data that can be sent or received. (e.g. if MTU=2K, can send
         requests up to bytes 2k and received responses up to 2k bytes).
 
-	IOCTL_MEI_NOTIFY_SET: enable or disable event notifications
+
+IOCTL_MEI_NOTIFY_SET
+---------------------
+Enable or disable event notifications.
+
+
+.. code-block:: none
 
 	Usage:
+
 		uint32_t enable;
+
 		ioctl(fd, IOCTL_MEI_NOTIFY_SET, &enable);
 
-	Inputs:
+
 		uint32_t enable = 1;
 		or
 		uint32_t enable[disable] = 0;
 
 	Error returns:
+
+
 		EINVAL	Wrong IOCTL Number
 		ENODEV	Device  is not initialized or the client not connected
 		ENOMEM	Unable to allocate memory to client internal data.
 		EFAULT	Fatal Error (e.g. Unable to access user input data)
 		EOPNOTSUPP if the device doesn't support the feature
 
-	Notes:
+:Note:
 	The client must be connected in order to enable notification events
 
 
-	IOCTL_MEI_NOTIFY_GET : retrieve event
+IOCTL_MEI_NOTIFY_GET
+--------------------
+Retrieve event
+
+.. code-block:: none
 
 	Usage:
 		uint32_t event;
@@ -137,7 +163,7 @@ The Intel MEI Driver supports the following IOCTL commands:
 		EFAULT	Fatal Error (e.g. Unable to access user input data)
 		EOPNOTSUPP if the device doesn't support the feature
 
-	Notes:
+:Note:
 	The client must be connected and event notification has to be enabled
 	in order to receive an event
 
-- 
2.20.1


^ permalink raw reply related

* RE: [char-misc-next 3/7] mei: docs: update mei documentation
From: Winkler, Tomas @ 2019-06-06 13:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Usyskin, Alexander, linux-kernel@vger.kernel.org, Jonathan Corbet,
	linux-doc@vger.kernel.org
In-Reply-To: <20190606131633.GA6083@kroah.com>



> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, June 06, 2019 16:17
> To: Winkler, Tomas <tomas.winkler@intel.com>
> Cc: Usyskin, Alexander <alexander.usyskin@intel.com>; linux-
> kernel@vger.kernel.org; Jonathan Corbet <corbet@lwn.net>; linux-
> doc@vger.kernel.org
> Subject: Re: [char-misc-next 3/7] mei: docs: update mei documentation
> 
> On Mon, Jun 03, 2019 at 12:14:02PM +0300, Tomas Winkler wrote:
> > The mei driver went via multiple changes, update the documentation and
> > fix formatting.
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >  Documentation/driver-api/mei/mei.rst | 96
> > ++++++++++++++++++----------
> >  1 file changed, 61 insertions(+), 35 deletions(-)
> 
> This patch is corrupted and can not apply.  Did you try to edit it by hand after
> generating it?

I have a script that strips some internal metadata, now I see it has some issues with 'Notes:' keywords. 
I've checked sand only this patch is affected. 

> Can you resend it alone?
On the way.
Thanks
Tomas


^ permalink raw reply

* Re: [char-misc-next 3/7] mei: docs: update mei documentation
From: Greg Kroah-Hartman @ 2019-06-06 13:16 UTC (permalink / raw)
  To: Tomas Winkler; +Cc: Alexander Usyskin, linux-kernel, Jonathan Corbet, linux-doc
In-Reply-To: <20190603091406.28915-4-tomas.winkler@intel.com>

On Mon, Jun 03, 2019 at 12:14:02PM +0300, Tomas Winkler wrote:
> The mei driver went via multiple changes, update
> the documentation and fix formatting.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  Documentation/driver-api/mei/mei.rst | 96 ++++++++++++++++++----------
>  1 file changed, 61 insertions(+), 35 deletions(-)

This patch is corrupted and can not apply.  Did you try to edit it by
hand after generating it?

Can you resend it alone?

thanks,

greg k-h

^ permalink raw reply

* RE: [PATCH v2 0/5] stm32-ddr-pmu driver creation
From: Gerald BAEZA @ 2019-06-06 12:14 UTC (permalink / raw)
  To: will.deacon@arm.com, mark.rutland@arm.com, robh+dt@kernel.org,
	mcoquelin.stm32@gmail.com, Alexandre TORGUE, corbet@lwn.net,
	linux@armlinux.org.uk, olof@lixom.net, horms+renesas@verge.net.au,
	arnd@arndb.de
  Cc: linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
In-Reply-To: <1558366019-24214-1-git-send-email-gerald.baeza@st.com>

Dear all

A gentle reminder to get your feedbacks on the series below.

Best regards

Gérald



> -----Original Message-----
> From: Gerald BAEZA <gerald.baeza@st.com>
> Sent: lundi 20 mai 2019 17:27
> To: will.deacon@arm.com; mark.rutland@arm.com; robh+dt@kernel.org;
> mcoquelin.stm32@gmail.com; Alexandre TORGUE
> <alexandre.torgue@st.com>; corbet@lwn.net; linux@armlinux.org.uk;
> olof@lixom.net; horms+renesas@verge.net.au; arnd@arndb.de
> Cc: linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-
> stm32@st-md-mailman.stormreply.com; linux-kernel@vger.kernel.org;
> linux-doc@vger.kernel.org; Gerald BAEZA <gerald.baeza@st.com>
> Subject: [PATCH v2 0/5] stm32-ddr-pmu driver creation
> 
> The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1
> SOC.
> 
> This series adds support for the DDRPERFM via a new stm32-ddr-pmu driver,
> registered into the perf framework.
> 
> This driver is inspired from arch/arm/mm/cache-l2x0-pmu.c
> 
> ---
> Changes from v1:
> - add 'resets' description (bindings) and using (driver). Thanks Rob.
> - rebase on 5.2-rc1 (that includes the ddrperfm clock control patch).
> 
> Gerald Baeza (5):
>   Documentation: perf: stm32: ddrperfm support
>   dt-bindings: perf: stm32: ddrperfm support
>   perf: stm32: ddrperfm driver creation
>   ARM: configs: enable STM32_DDR_PMU
>   ARM: dts: stm32: add ddrperfm on stm32mp157c
> 
>  .../devicetree/bindings/perf/stm32-ddr-pmu.txt     |  20 +
>  Documentation/perf/stm32-ddr-pmu.txt               |  41 ++
>  arch/arm/boot/dts/stm32mp157c.dtsi                 |   9 +
>  arch/arm/configs/multi_v7_defconfig                |   1 +
>  drivers/perf/Kconfig                               |   6 +
>  drivers/perf/Makefile                              |   1 +
>  drivers/perf/stm32_ddr_pmu.c                       | 512 +++++++++++++++++++++
>  7 files changed, 590 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/perf/stm32-ddr-
> pmu.txt
>  create mode 100644 Documentation/perf/stm32-ddr-pmu.txt
>  create mode 100644 drivers/perf/stm32_ddr_pmu.c
> 
> --
> 2.7.4

^ permalink raw reply

* Re: [PATCH v3 0/2] ima/evm fixes for v5.2
From: Roberto Sassu @ 2019-06-06 11:43 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, mjg59
  Cc: linux-integrity, linux-security-module, linux-doc, stable,
	linux-kernel, silviu.vlasceanu
In-Reply-To: <20190606112620.26488-1-roberto.sassu@huawei.com>

On 6/6/2019 1:26 PM, Roberto Sassu wrote:
> Previous versions included the patch 'ima: don't ignore INTEGRITY_UNKNOWN
> EVM status'. However, I realized that this patch cannot be accepted alone
> because IMA-Appraisal would deny access to new files created during the
> boot. With the current behavior, those files are accessible because they
> have a valid security.ima (not protected by EVM) created after the first
> write.
> 
> A solution for this problem is to initialize EVM very early with a random
> key. Access to created files will be granted, even with the strict
> appraisal, because after the first write those files will have both
> security.ima and security.evm (HMAC calculated with the random key).
> 
> Strict appraisal will work only if it is done with signatures until the
> persistent HMAC key is loaded.

Changelog

v2:
- remove patch 1/3 (evm: check hash algorithm passed to init_desc());
   already accepted
- remove patch 3/3 (ima: show rules with IMA_INMASK correctly);
   already accepted
- add new patch (evm: add option to set a random HMAC key at early boot)
- patch 2/3: modify patch description

v1:
- remove patch 2/4 (evm: reset status in evm_inode_post_setattr()); file
   attributes cannot be set if the signature is portable and immutable
- patch 3/4: add __ro_after_init to ima_appraise_req_evm variable
   declaration
- patch 3/4: remove ima_appraise_req_evm kernel option and introduce
   'enforce-evm' and 'log-evm' as possible values for ima_appraise=
- remove patch 4/4 (ima: only audit failed appraisal verifications)
- add new patch (ima: show rules with IMA_INMASK correctly)


> Roberto Sassu (2):
>    evm: add option to set a random HMAC key at early boot
>    ima: add enforce-evm and log-evm modes to strictly check EVM status
> 
>   .../admin-guide/kernel-parameters.txt         | 11 ++--
>   security/integrity/evm/evm.h                  | 10 +++-
>   security/integrity/evm/evm_crypto.c           | 57 ++++++++++++++++---
>   security/integrity/evm/evm_main.c             | 41 ++++++++++---
>   security/integrity/ima/ima_appraise.c         |  8 +++
>   security/integrity/integrity.h                |  1 +
>   6 files changed, 106 insertions(+), 22 deletions(-)
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply

* [PATCH v3 2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status
From: Roberto Sassu @ 2019-06-06 11:26 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, mjg59
  Cc: linux-integrity, linux-security-module, linux-doc, stable,
	linux-kernel, silviu.vlasceanu, Roberto Sassu
In-Reply-To: <20190606112620.26488-1-roberto.sassu@huawei.com>

IMA and EVM have been designed as two independent subsystems: the first for
checking the integrity of file data; the second for checking file metadata.
Making them independent allows users to adopt them incrementally.

The point of intersection is in IMA-Appraisal, which calls
evm_verifyxattr() to ensure that security.ima wasn't modified during an
offline attack. The design choice, to ensure incremental adoption, was to
continue appraisal verification if evm_verifyxattr() returns
INTEGRITY_UNKNOWN. This value is returned when EVM is not enabled in the
kernel configuration, or if the HMAC key has not been loaded yet.

Although this choice appears legitimate, it might not be suitable for
hardened systems, where the administrator expects that access is denied if
there is any error. An attacker could intentionally delete the EVM keys
from the system and set the file digest in security.ima to the actual file
digest so that the final appraisal status is INTEGRITY_PASS.

This patch allows such hardened systems to strictly enforce an access
control policy based on the validity of signatures/HMACs, by introducing
two new values for the ima_appraise= kernel option: enforce-evm and
log-evm.

Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
 Documentation/admin-guide/kernel-parameters.txt | 3 ++-
 security/integrity/ima/ima_appraise.c           | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index fe5cde58c11b..0585194ca736 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1587,7 +1587,8 @@
 			Set number of hash buckets for inode cache.
 
 	ima_appraise=	[IMA] appraise integrity measurements
-			Format: { "off" | "enforce" | "fix" | "log" }
+			Format: { "off" | "enforce" | "fix" | "log" |
+				  "enforce-evm" | "log-evm" }
 			default: "enforce"
 
 	ima_appraise_tcb [IMA] Deprecated.  Use ima_policy= instead.
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 5fb7127bbe68..afef06e10fb9 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -18,6 +18,7 @@
 
 #include "ima.h"
 
+static bool ima_appraise_req_evm __ro_after_init;
 static int __init default_appraise_setup(char *str)
 {
 #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
@@ -28,6 +29,9 @@ static int __init default_appraise_setup(char *str)
 	else if (strncmp(str, "fix", 3) == 0)
 		ima_appraise = IMA_APPRAISE_FIX;
 #endif
+	if (strcmp(str, "enforce-evm") == 0 ||
+	    strcmp(str, "log-evm") == 0)
+		ima_appraise_req_evm = true;
 	return 1;
 }
 
@@ -245,7 +249,11 @@ int ima_appraise_measurement(enum ima_hooks func,
 	switch (status) {
 	case INTEGRITY_PASS:
 	case INTEGRITY_PASS_IMMUTABLE:
+		break;
 	case INTEGRITY_UNKNOWN:
+		if (ima_appraise_req_evm &&
+		    xattr_value->type != EVM_IMA_XATTR_DIGSIG)
+			goto out;
 		break;
 	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
 	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
-- 
2.17.1


^ permalink raw reply related

* [PATCH v3 1/2] evm: add option to set a random HMAC key at early boot
From: Roberto Sassu @ 2019-06-06 11:26 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, mjg59
  Cc: linux-integrity, linux-security-module, linux-doc, stable,
	linux-kernel, silviu.vlasceanu, Roberto Sassu
In-Reply-To: <20190606112620.26488-1-roberto.sassu@huawei.com>

Mutable files can be created before the HMAC key is unsealed, for example
the dracut state and the systemd journal. Next accesses to those files will
be denied if the new appraisal mode enforce-evm is selected
(INTEGRITY_UNKNOWN returned by EVM is considered as an error).

This patch solves this problem by initializing EVM at early boot with a
randomly generated key. This key is used to calculate and verify the HMAC
for new files in a tmpfs filesystem, until the persistent key is loaded.

The new xattr type EVM_XATTR_HMAC_RND_KEY has been introduced to determine
which key should be used to verify the HMAC. This type is used for new
files and file updates (unless security.evm exists with a different type),
until the persistent key is loaded. Afterwards, existing HMACs calculated
with the random key are replaced with HMACs calculated with the persistent
key.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 .../admin-guide/kernel-parameters.txt         |  8 ++-
 security/integrity/evm/evm.h                  | 10 +++-
 security/integrity/evm/evm_crypto.c           | 57 ++++++++++++++++---
 security/integrity/evm/evm_main.c             | 41 ++++++++++---
 security/integrity/integrity.h                |  1 +
 5 files changed, 96 insertions(+), 21 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 138f6664b2e2..fe5cde58c11b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1239,9 +1239,11 @@
 			has equivalent usage. See its documentation for details.
 
 	evm=		[EVM]
-			Format: { "fix" }
-			Permit 'security.evm' to be updated regardless of
-			current integrity status.
+			Format: { "fix" | "random" }
+			Specify "fix" to permit 'security.evm' to be updated
+			regardless of current integrity status. Specify "random"
+			to initialize EVM with a random key to be used for new
+			files until the persistent HMAC key is loaded.
 
 	failslab=
 	fail_page_alloc=
diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index c3f437f5db10..0ca4490b7e40 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -24,9 +24,11 @@
 #define EVM_INIT_HMAC	0x0001
 #define EVM_INIT_X509	0x0002
 #define EVM_ALLOW_METADATA_WRITES	0x0004
+#define EVM_INIT_HMAC_RND_KEY	0x0008
 #define EVM_SETUP_COMPLETE 0x80000000 /* userland has signaled key load */
 
-#define EVM_KEY_MASK (EVM_INIT_HMAC | EVM_INIT_X509)
+#define EVM_PERSISTENT_KEY_MASK (EVM_INIT_HMAC | EVM_INIT_X509)
+#define EVM_KEY_MASK (EVM_INIT_HMAC | EVM_INIT_X509 | EVM_INIT_HMAC_RND_KEY)
 #define EVM_INIT_MASK (EVM_INIT_HMAC | EVM_INIT_X509 | EVM_SETUP_COMPLETE | \
 		       EVM_ALLOW_METADATA_WRITES)
 
@@ -53,19 +55,21 @@ struct evm_digest {
 } __packed;
 
 int evm_init_key(void);
+void evm_set_random_key(void);
 int evm_update_evmxattr(struct dentry *dentry,
 			const char *req_xattr_name,
 			const char *req_xattr_value,
 			size_t req_xattr_value_len);
 int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value,
-		  size_t req_xattr_value_len, struct evm_digest *data);
+		  size_t req_xattr_value_len, char type,
+		  struct evm_digest *data);
 int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value,
 		  size_t req_xattr_value_len, char type,
 		  struct evm_digest *data);
 int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
-		  char *hmac_val);
+		  struct evm_ima_xattr_data *evm_xattr);
 int evm_init_secfs(void);
 
 #endif
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 82a38e801ee4..51a02200b057 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -19,6 +19,7 @@
 #include <linux/crypto.h>
 #include <linux/xattr.h>
 #include <linux/evm.h>
+#include <linux/random.h>
 #include <keys/encrypted-type.h>
 #include <crypto/hash.h>
 #include <crypto/hash_info.h>
@@ -30,6 +31,7 @@ static unsigned char evmkey[MAX_KEY_SIZE];
 static const int evmkey_len = MAX_KEY_SIZE;
 
 struct crypto_shash *hmac_tfm;
+struct crypto_shash *hmac_rnd_tfm;
 static struct crypto_shash *evm_tfm[HASH_ALGO__LAST];
 
 static DEFINE_MUTEX(mutex);
@@ -62,8 +64,10 @@ int evm_set_key(void *key, size_t keylen)
 	rc = -EINVAL;
 	if (keylen > MAX_KEY_SIZE)
 		goto inval;
+	memset(evmkey, 0, sizeof(evmkey));
 	memcpy(evmkey, key, keylen);
 	evm_initialized |= EVM_INIT_HMAC;
+	evm_initialized &= ~EVM_INIT_HMAC_RND_KEY;
 	pr_info("key initialized\n");
 	return 0;
 inval:
@@ -74,6 +78,12 @@ int evm_set_key(void *key, size_t keylen)
 }
 EXPORT_SYMBOL_GPL(evm_set_key);
 
+void evm_set_random_key(void)
+{
+	get_random_bytes(evmkey, sizeof(evmkey));
+	evm_initialized |= EVM_INIT_HMAC_RND_KEY;
+}
+
 static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 {
 	long rc;
@@ -88,6 +98,9 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 		}
 		tfm = &hmac_tfm;
 		algo = evm_hmac;
+	} else if (type == EVM_XATTR_HMAC_RND_KEY) {
+		tfm = &hmac_rnd_tfm;
+		algo = evm_hmac;
 	} else {
 		if (hash_algo >= HASH_ALGO__LAST)
 			return ERR_PTR(-EINVAL);
@@ -108,7 +121,7 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 			mutex_unlock(&mutex);
 			return ERR_PTR(rc);
 		}
-		if (type == EVM_XATTR_HMAC) {
+		if (type == EVM_XATTR_HMAC || EVM_XATTR_HMAC_RND_KEY) {
 			rc = crypto_shash_setkey(*tfm, evmkey, evmkey_len);
 			if (rc) {
 				crypto_free_shash(*tfm);
@@ -255,10 +268,10 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 
 int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value, size_t req_xattr_value_len,
-		  struct evm_digest *data)
+		  char type, struct evm_digest *data)
 {
 	return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
-				    req_xattr_value_len, EVM_XATTR_HMAC, data);
+				    req_xattr_value_len, type, data);
 }
 
 int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
@@ -296,6 +309,29 @@ static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
 	return rc;
 }
 
+static enum evm_ima_xattr_type evm_get_default_type(struct dentry *dentry)
+{
+	enum evm_ima_xattr_type evm_default_type = EVM_XATTR_HMAC;
+	struct evm_ima_xattr_data xattr_data;
+	int rc;
+
+	if (evm_initialized & EVM_INIT_HMAC_RND_KEY)
+		evm_default_type = EVM_XATTR_HMAC_RND_KEY;
+	else
+		goto out;
+
+	rc = vfs_getxattr(dentry, XATTR_NAME_EVM, (char *)&xattr_data,
+			  sizeof(xattr_data));
+
+	if (rc == sizeof(xattr_data))
+		evm_default_type = xattr_data.type;
+out:
+	if (evm_default_type != EVM_XATTR_HMAC_RND_KEY &&
+	    !(evm_initialized & EVM_INIT_HMAC))
+		return IMA_XATTR_LAST;
+
+	return evm_default_type;
+}
 
 /*
  * Calculate the hmac and update security.evm xattr
@@ -306,6 +342,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 			const char *xattr_value, size_t xattr_value_len)
 {
 	struct inode *inode = d_backing_inode(dentry);
+	enum evm_ima_xattr_type evm_default_type;
 	struct evm_digest data;
 	int rc = 0;
 
@@ -319,11 +356,15 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 	if (rc)
 		return -EPERM;
 
+	evm_default_type = evm_get_default_type(dentry);
+	if (evm_default_type == IMA_XATTR_LAST)
+		return -ENOKEY;
+
 	data.hdr.algo = HASH_ALGO_SHA1;
 	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
-			   xattr_value_len, &data);
+			   xattr_value_len, evm_default_type, &data);
 	if (rc == 0) {
-		data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
+		data.hdr.xattr.sha1.type = evm_default_type;
 		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
 					   &data.hdr.xattr.data[1],
 					   SHA1_DIGEST_SIZE + 1, 0);
@@ -334,18 +375,18 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 }
 
 int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
-		  char *hmac_val)
+		  struct evm_ima_xattr_data *evm_xattr)
 {
 	struct shash_desc *desc;
 
-	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
+	desc = init_desc(evm_xattr->type, HASH_ALGO_SHA1);
 	if (IS_ERR(desc)) {
 		pr_info("init_desc failed\n");
 		return PTR_ERR(desc);
 	}
 
 	crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len);
-	hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
+	hmac_add_misc(desc, inode, evm_xattr->type, evm_xattr->digest);
 	kfree(desc);
 	return 0;
 }
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index b6d9f14bc234..faa4a02a3139 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -59,14 +59,16 @@ static struct xattr_list evm_config_default_xattrnames[] = {
 
 LIST_HEAD(evm_config_xattrnames);
 
-static int evm_fixmode;
-static int __init evm_set_fixmode(char *str)
+static int evm_fixmode, evm_random_key;
+static int __init evm_setup(char *str)
 {
 	if (strncmp(str, "fix", 3) == 0)
 		evm_fixmode = 1;
+	if (strncmp(str, "random", 6) == 0)
+		evm_random_key = 1;
 	return 0;
 }
-__setup("evm=", evm_set_fixmode);
+__setup("evm=", evm_setup);
 
 static void __init evm_init_config(void)
 {
@@ -92,6 +94,11 @@ static bool evm_key_loaded(void)
 	return (bool)(evm_initialized & EVM_KEY_MASK);
 }
 
+static bool evm_persistent_key_loaded(void)
+{
+	return (bool)(evm_initialized & EVM_PERSISTENT_KEY_MASK);
+}
+
 static int evm_find_protected_xattrs(struct dentry *dentry)
 {
 	struct inode *inode = d_backing_inode(dentry);
@@ -152,7 +159,9 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 				GFP_NOFS);
 	if (rc <= 0) {
 		evm_status = INTEGRITY_FAIL;
-		if (rc == -ENODATA) {
+		if (!evm_persistent_key_loaded()) {
+			evm_status = INTEGRITY_UNKNOWN;
+		} else if (rc == -ENODATA) {
 			rc = evm_find_protected_xattrs(dentry);
 			if (rc > 0)
 				evm_status = INTEGRITY_NOLABEL;
@@ -164,11 +173,18 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		goto out;
 	}
 
+	if (xattr_data->type != EVM_XATTR_HMAC_RND_KEY &&
+	    !evm_persistent_key_loaded()) {
+		evm_status = INTEGRITY_UNKNOWN;
+		goto out;
+	}
+
 	xattr_len = rc;
 
 	/* check value type */
 	switch (xattr_data->type) {
 	case EVM_XATTR_HMAC:
+	case EVM_XATTR_HMAC_RND_KEY:
 		if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
 			evm_status = INTEGRITY_FAIL;
 			goto out;
@@ -176,7 +192,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 
 		digest.hdr.algo = HASH_ALGO_SHA1;
 		rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
-				   xattr_value_len, &digest);
+				   xattr_value_len, xattr_data->type, &digest);
 		if (rc)
 			break;
 		rc = crypto_memneq(xattr_data->digest, digest.digest,
@@ -523,18 +539,26 @@ int evm_inode_init_security(struct inode *inode,
 				 const struct xattr *lsm_xattr,
 				 struct xattr *evm_xattr)
 {
+	enum evm_ima_xattr_type evm_default_type = EVM_XATTR_HMAC;
 	struct evm_ima_xattr_data *xattr_data;
 	int rc;
 
 	if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name))
 		return 0;
 
+	if (!evm_persistent_key_loaded()) {
+		if (inode->i_sb->s_magic != TMPFS_MAGIC)
+			return 0;
+
+		evm_default_type = EVM_XATTR_HMAC_RND_KEY;
+	}
+
 	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
 	if (!xattr_data)
 		return -ENOMEM;
 
-	xattr_data->type = EVM_XATTR_HMAC;
-	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
+	xattr_data->type = evm_default_type;
+	rc = evm_init_hmac(inode, lsm_xattr, xattr_data);
 	if (rc < 0)
 		goto out;
 
@@ -584,6 +608,9 @@ static int __init init_evm(void)
 		}
 	}
 
+	if (!error && evm_random_key)
+		evm_set_random_key();
+
 	return error;
 }
 
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 7de59f44cba3..a037d10db46f 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -74,6 +74,7 @@ enum evm_ima_xattr_type {
 	EVM_IMA_XATTR_DIGSIG,
 	IMA_XATTR_DIGEST_NG,
 	EVM_XATTR_PORTABLE_DIGSIG,
+	EVM_XATTR_HMAC_RND_KEY,
 	IMA_XATTR_LAST
 };
 
-- 
2.17.1


^ permalink raw reply related

* [PATCH v3 0/2] ima/evm fixes for v5.2
From: Roberto Sassu @ 2019-06-06 11:26 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, mjg59
  Cc: linux-integrity, linux-security-module, linux-doc, stable,
	linux-kernel, silviu.vlasceanu, Roberto Sassu

Previous versions included the patch 'ima: don't ignore INTEGRITY_UNKNOWN
EVM status'. However, I realized that this patch cannot be accepted alone
because IMA-Appraisal would deny access to new files created during the
boot. With the current behavior, those files are accessible because they
have a valid security.ima (not protected by EVM) created after the first
write.

A solution for this problem is to initialize EVM very early with a random
key. Access to created files will be granted, even with the strict
appraisal, because after the first write those files will have both
security.ima and security.evm (HMAC calculated with the random key).

Strict appraisal will work only if it is done with signatures until the
persistent HMAC key is loaded.


Roberto Sassu (2):
  evm: add option to set a random HMAC key at early boot
  ima: add enforce-evm and log-evm modes to strictly check EVM status

 .../admin-guide/kernel-parameters.txt         | 11 ++--
 security/integrity/evm/evm.h                  | 10 +++-
 security/integrity/evm/evm_crypto.c           | 57 ++++++++++++++++---
 security/integrity/evm/evm_main.c             | 41 ++++++++++---
 security/integrity/ima/ima_appraise.c         |  8 +++
 security/integrity/integrity.h                |  1 +
 6 files changed, 106 insertions(+), 22 deletions(-)

-- 
2.17.1


^ permalink raw reply

* Re: [PATCH v2] Add a document on rebasing and merging
From: Jani Nikula @ 2019-06-06  9:12 UTC (permalink / raw)
  To: Jonathan Corbet, linux-doc
  Cc: LKML, Linus Torvalds, Theodore Ts'o, Geert Uytterhoeven,
	David Rientjes
In-Reply-To: <20190604134835.16fc6bfa@lwn.net>

On Tue, 04 Jun 2019, Jonathan Corbet <corbet@lwn.net> wrote:
> Every merge window seems to involve at least one episode where subsystem
> maintainers don't manage their trees as Linus would like.  Document the
> expectations so that at least he has something to point people to.

Good stuff. Some notes inline.

BR,
Jani.

>
> Acked-by: David Rientjes <rientjes@google.com>
> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> ---
> Changes in v2:
>   - Try to clear up "reparenting" v. "history modification"
>   - Make the "don't rebase public branches" rule into more of a guideline
>   - Fix typos noted by Geert
>   - Rename the document to better reflect its contents
>
>  Documentation/maintainer/index.rst            |   1 +
>  .../maintainer/rebasing-and-merging.rst       | 216 ++++++++++++++++++
>  2 files changed, 217 insertions(+)
>  create mode 100644 Documentation/maintainer/rebasing-and-merging.rst
>
> diff --git a/Documentation/maintainer/index.rst
> b/Documentation/maintainer/index.rst index 2a14916930cb..56e2c09dfa39
> 100644 --- a/Documentation/maintainer/index.rst
> +++ b/Documentation/maintainer/index.rst
> @@ -10,5 +10,6 @@ additions to this manual.
>     :maxdepth: 2
>  
>     configure-git
> +   rebasing-and-merging
>     pull-requests
>  
> diff --git a/Documentation/maintainer/rebasing-and-merging.rst
> b/Documentation/maintainer/rebasing-and-merging.rst new file mode 100644
> index 000000000000..2987bd45dfb2
> --- /dev/null
> +++ b/Documentation/maintainer/rebasing-and-merging.rst
> @@ -0,0 +1,216 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +====================
> +Rebasing and merging
> +====================
> +
> +Maintaining a subsystem, as a general rule, requires a familiarity with
> the +Git source-code management system.  Git is a powerful tool with a lot
> of +features; as is often the case with such tools, there are right and
> wrong +ways to use those features.  This document looks in particular at
> the use +of rebasing and merging.  Maintainers often get in trouble when
> they use +those tools incorrectly, but avoiding problems is not actually
> all that +hard.
> +
> +One thing to be aware of in general is that, unlike many other projects,
> +the kernel community is not scared by seeing merge commits in its
> +development history.  Indeed, given the scale of the project, avoiding
> +merges would be nearly impossible.  Some problems encountered by
> +maintainers result from a desire to avoid merges, while others come from
> +merging a little too often.
> +
> +Rebasing
> +========
> +
> +"Rebasing" is the process of changing the history of a series of commits
> +within a repository.  There are two different types of operations that are
> +referred to as rebasing since both are done with the ``git rebase``
> +command, but there are significant differences between them:
> +
> + - Rebasing can change the parent (starting) commit upon which a series of
> +   patches is built.  For example, a rebase operation could take a patch
> +   set built on the previous kernel release and base it, instead, on the
> +   current release.  We'll call this operation "reparenting" in the
> +   discussion below.
> +
> + - Changing the history of a set of patches by fixing (or deleting) broken
> +   commits, adding patches, adding tags to commit changelogs, or changing
> +   the order in which commits are applied.  In the following text, this
> +   type of operation will be referred to as "history modification"
> +
> +The term "rebasing" will be used to refer to both of the above operations.
> +Used properly, rebasing can yield a cleaner and clearer development
> +history; used improperly, it can obscure that history and introduce bugs.
> +
> +There are a few rules of thumb that can help developers to avoid the worst
> +perils of rebasing:
> +
> + - History that has been exposed to the world beyond your private system
> +   should usually not be changed.  Others may have pulled a copy of your
> +   tree and built on it; modifying your tree will create pain for them.
> If
> +   work is in need of rebasing, that is usually a sign that it is not yet
> +   ready to be committed to a public repository.
> +
> +   That said, there are always exceptions.  Some trees (linux-next being
> +   a significant example) are frequently rebased by their nature, and
> +   developers know not to base work on them.  Developers will sometimes
> +   expose an unstable branch for others to test with or for automated
> +   testing services.  If you do expose a branch that may be unstable in
> +   this way, be sure that prospective users know not to base work on it.
> +
> + - Do not rebase a branch that contains history created by others.  If you
> +   have pulled changes from another developer's repository, you are now a
> +   custodian of their history.  You should not change it.  With few
> +   exceptions, for example, a broken commit in a tree like this should be
> +   explicitly reverted rather than disappeared via history modification.
> +
> + - Do not reparent a tree without a good reason to do so.  Just being on a
> +   newer base or avoiding a merge with an upstream repository is not
> +   generally a good reason.
> +
> + - If you must reparent a repository, do not pick some random kernel
> commit
> +   as the new base.  The kernel is often in a relatively unstable state
> +   between release points; basing development on one of those points
> +   increases the chances of running into surprising bugs.  When a patch
> +   series must move to a new base, pick a stable point (such as one of
> +   the -rc releases) to move to.
> +
> + - Realize that the rebasing a patch series changes the environment in
> +   which it was developed and, likely, invalidates much of the testing
> that
> +   was done.  A rebased patch series should, as a general rule, be treated
> +   like new code and retested from the beginning.
> +
> +A frequent cause of merge-window trouble is when Linus is presented with a
> +patch series that has clearly been reparented, often to a random commit,
> +shortly before the pull request was sent.  The chances of such a series
> +having been adequately tested are relatively low - as are the chances of
> +the pull request being acted upon.
> +
> +If, instead, rebasing is limited to private trees, commits are based on a
> +well-known starting point, and they are well tested, the potential for
> +trouble is low.
> +
> +Merging
> +=======
> +
> +Merging is a common operation in the kernel development process; the 5.1
> +development cycle included 1,126 merge commits - nearly 9% of the total.
> +Kernel work is accumulated in over 100 different subsystem trees, each of
> +which may contain multiple topic branches; each branch is usually
> developed +independently of the others.  So naturally, at least merge will
> be required +before any given branch finds its way into an upstream
> repository. +
> +Many projects require that branches in pull requests be based on the
> +current trunk so that no merge commits appear in the history.  The kernel
> +is not such a project; any rebasing of branches to avoid merges will, as
> +described above, lead to certain trouble.
> +
> +Subsystem maintainers find themselves having to do two types of merges:
> +from lower-level subsystem trees and from others, either sibling trees or
> +the mainline.  The best practices to follow differ in those two
> situations. +
> +Merging from lower-level trees
> +------------------------------
> +
> +Larger subsystems tend to have multiple levels of maintainers, with the
> +lower-level maintainers sending pull requests to the higher levels.
> Acting +on such a pull request will almost certainly generate a merge
> commit; that +is as it should be.  In fact, subsystem maintainers may want
> to use +the --no-ff flag to force the addition of a merge commit in the
> rare cases +where one would not normally be created so that the reasons
> for the merge +can be recorded.  The changelog for the merge should, for
> any kind of +merge, say *why* the merge is being done.  For a lower-level
> tree, "why" is +usually a summary of the changes that will come with that
> pull. +
> +Maintainers at all levels should be using signed tags on their pull
> +requests, and upstream maintainers should verify the tags when pulling
> +branches.  Failure to do so threatens the security of the development
> +process as a whole.
> +
> +As per the rules outlined above, once you have merged somebody else's
> +history into your tree, you cannot rebase that branch, even if you
> +otherwise would be able to.
> +
> +Merging from sibling or upstream trees
> +--------------------------------------
> +
> +While merges from downstream are common and unremarkable, merges from
> other +trees tend to be a red flag when it comes time to push a branch
> upstream. +Such merges need to be carefully thought about and well
> justified, or +there's a good chance that a subsequent pull request will
> be rejected. +
> +It is natural to want to merge the master branch into a repository; it can
> +help to make sure that there are no conflicts with parallel development
> and +generally gives a warm, fuzzy feeling of being up-to-date.  But this
> +temptation should be avoided almost all of the time.
> +
> +Why is that?  Merges with upstream will muddy the development history of
> +your own branch.  They will significantly increase your chances of
> +encountering bugs from elsewhere in the community and make it hard to
> +ensure that the work you are managing is stable and ready for upstream.
> +Frequent merges can also obscure problems with the development process in
> +your tree; they can hide interactions with other trees that should not be
> +happening (often) in a well-managed branch.
> +
> +One of the most frequent causes of merge-related trouble is when a
> +maintainer merges with the upstream in order to resolve merge conflicts
> +before sending a pull request.  Again, this temptation is easy enough to
> +understand, but it should absolutely be avoided.  This is especially true
> +for the final pull request: Linus is adamant that he would much rather see
> +merge conflicts than unnecessary back merges.  Seeing the conflicts lets

I think "backmerge" as a term deserves to be highlighted in the heading
or first paragraph of the section.

Occasionally backmerges are required. As a rule of thumb, it might be
worth mentioning you probably shouldn't do such merges across subsystem
hierarchies, i.e. ask the level above you to do a backmerge first, and
then backmerge from them. And that when backmerging from Linus' tree,
the merge point should be a tag.

> +him know where potential problem areas are.  He does a lot of merges (382
> +in the 5.1 development cycle) and has gotten quite good at conflict
> +resolution - often better than the developers involved.
> +
> +So what should a maintainer do when there is a conflict between their
> +subsystem branch and the mainline?  The most important step is to warn
> +Linus in the pull request that the conflict will happen; if nothing else,
> +that demonstrates an awareness of how your branch fits into the whole.
> For +especially difficult conflicts, create and push a *separate* branch
> to show +how you would resolve things.  Mention that branch in your pull
> request, +but the pull request itself should be for the unmerged branch.
> +
> +Even in the absence of known conflicts, doing a test merge before sending
> a +pull request is a good idea.  It may alert you to problems that you
> somehow +didn't see from linux-next and helps to understand exactly what
> you are +asking upstream to do.
> +
> +Another reason for doing merges of upstream or another subsystem tree is
> to +resolve dependencies.  These dependency issues do happen at times, and
> +sometimes a cross-merge with another tree is the best way to resolve them;
> +as always, in such situations, the merge commit should explain why the
> +merge has been done.  Take a moment to do it right; people will read those
> +changelogs.
> +
> +Often, though, dependency issues indicate that a change of approach is
> +needed.  Merging another subsystem tree to resolve a dependency risks
> +bringing in other bugs.  If that subsystem tree fails to be pulled
> +upstream, whatever problems it had will block the merging of your tree as
> +well.  Possible alternatives include agreeing with the maintainer to carry
> +both sets of changes in one of the trees or creating a special branch
> +dedicated to the dependent commits.  If the dependency is related to major
> +infrastructural changes, the right solution might be to hold the dependent
> +commits for one development cycle so that those changes have time to
> +stabilize in the mainline.

Is it not a common convention to call these special branches "topic
branches"?

FWIW, I don't think I've ever done a cross-merge or a direct merge from
a sibling tree. I've always solved the cases either by topic branches
merged to both trees or by having both trees merged to the first common
upstream tree, and then backmerging. From my POV feels like these
solutions should be presented more prominently than cross-merges.

> +
> +Finally
> +=======
> +
> +It is relatively common to merge with the mainline toward the beginning of
> +the development cycle in order to pick up changes and fixes done elsewhere
> +in the tree.  As always, such a merge should pick a well-known release
> +point rather than some random spot.  If your upstream-bound branch has
> +emptied entirely into the mainline during the merge window, you can pull
> it +forward with a command like::
> +
> +  git merge v5.2-rc1^0
> +
> +The "^0" will cause Git to do a fast-forward merge (which should be
> +possible in this situation), thus avoiding the addition of a spurious
> merge +commit.
> +
> +The guidelines laid out above are just that: guidelines.  There will
> always +be situations that call out for a different solution, and these
> guidelines +should not prevent developers from doing the right thing when
> the need +arises.  But one should always think about whether the need has
> truly +arisen and be prepared to explain why something abnormal needs to
> be done. -- 
> 2.21.0
>

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply

* Re: [PATCH 1/2] Documentation/i915: Fix kernel-doc references to moved gem files
From: Jani Nikula @ 2019-06-06  8:23 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx; +Cc: linux-doc, Chris Wilson
In-Reply-To: <87d0jss4uo.fsf@gaia.fi.intel.com>

On Wed, 05 Jun 2019, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> Jani Nikula <jani.nikula@intel.com> writes:
>
>> The error messages could be more descriptive, but fix these caused by
>> file moves:
>>
>> WARNING: kernel-doc './scripts/kernel-doc -rst -enable-lineno -internal
>>     ./drivers/gpu/drm/i915/i915_gem_shrinker.c' failed with return code 2
>> WARNING: kernel-doc './scripts/kernel-doc -rst -enable-lineno -function
>>     User command execution ./drivers/gpu/drm/i915/i915_gem_execbuffer.c'
>>     failed with return code 1
>> WARNING: kernel-doc './scripts/kernel-doc -rst -enable-lineno -internal
>>     ./drivers/gpu/drm/i915/i915_gem_tiling.c' failed with return code 2
>> WARNING: kernel-doc './scripts/kernel-doc -rst -enable-lineno -function
>>     buffer object tiling ./drivers/gpu/drm/i915/i915_gem_tiling.c'
>>     failed with return code 1
>>
>> Fixes: 10be98a77c55 ("drm/i915: Move more GEM objects under gem/")
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> /o\
>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Thanks for the reviews, I took the liberty of pushing these to dinq with
just BAT results, as these are purely documentation/comment changes.

BR,
Jani.

>
>> ---
>>  Documentation/gpu/i915.rst | 11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
>> index 6c75380b2928..f98ee95da90f 100644
>> --- a/Documentation/gpu/i915.rst
>> +++ b/Documentation/gpu/i915.rst
>> @@ -349,7 +349,7 @@ of buffer object caches. Shrinking is used to make main memory
>>  available. Note that this is mostly orthogonal to evicting buffer
>>  objects, which has the goal to make space in gpu virtual address spaces.
>>  
>> -.. kernel-doc:: drivers/gpu/drm/i915/i915_gem_shrinker.c
>> +.. kernel-doc:: drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
>>     :internal:
>>  
>>  Batchbuffer Parsing
>> @@ -373,7 +373,7 @@ Batchbuffer Pools
>>  User Batchbuffer Execution
>>  --------------------------
>>  
>> -.. kernel-doc:: drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +.. kernel-doc:: drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>     :doc: User command execution
>>  
>>  Logical Rings, Logical Ring Contexts and Execlists
>> @@ -382,9 +382,6 @@ Logical Rings, Logical Ring Contexts and Execlists
>>  .. kernel-doc:: drivers/gpu/drm/i915/gt/intel_lrc.c
>>     :doc: Logical Rings, Logical Ring Contexts and Execlists
>>  
>> -.. kernel-doc:: drivers/gpu/drm/i915/gt/intel_lrc.c
>> -   :internal:
>> -
>>  Global GTT views
>>  ----------------
>>  
>> @@ -415,10 +412,10 @@ Hardware Tiling and Swizzling Details
>>  Object Tiling IOCTLs
>>  --------------------
>>  
>> -.. kernel-doc:: drivers/gpu/drm/i915/i915_gem_tiling.c
>> +.. kernel-doc:: drivers/gpu/drm/i915/gem/i915_gem_tiling.c
>>     :internal:
>>  
>> -.. kernel-doc:: drivers/gpu/drm/i915/i915_gem_tiling.c
>> +.. kernel-doc:: drivers/gpu/drm/i915/gem/i915_gem_tiling.c
>>     :doc: buffer object tiling
>>  
>>  WOPCM
>> -- 
>> 2.20.1

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply

* Re: [PATCH 2/3] treewide: trivial: fix s/poped/popped/ typo
From: Steven Rostedt @ 2019-06-06  6:47 UTC (permalink / raw)
  To: George G. Davis
  Cc: Jonathan Corbet, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Jiri Kosina, Masami Hiramatsu, Andi Kleen, Jann Horn, Nadav Amit,
	open list:DOCUMENTATION, open list
In-Reply-To: <1559766612-12178-2-git-send-email-george_davis@mentor.com>

On Wed, 5 Jun 2019 16:30:10 -0400
"George G. Davis" <george_davis@mentor.com> wrote:

> Fix a couple of s/poped/popped/ typos.
> 
> Cc: Jiri Kosina <trivial@kernel.org>
> Signed-off-by: George G. Davis <george_davis@mentor.com>

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> ---
>  Documentation/arm/mem_alignment | 2 +-
>  arch/x86/kernel/kprobes/core.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/arm/mem_alignment b/Documentation/arm/mem_alignment
> index 6335fcacbba9..e110e2781039 100644
> --- a/Documentation/arm/mem_alignment
> +++ b/Documentation/arm/mem_alignment
> @@ -1,4 +1,4 @@
> -Too many problems poped up because of unnoticed misaligned memory access in
> +Too many problems popped up because of unnoticed misaligned memory access in
>  kernel code lately.  Therefore the alignment fixup is now unconditionally
>  configured in for SA11x0 based targets.  According to Alan Cox, this is a
>  bad idea to configure it out, but Russell King has some good reasons for
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 6afd8061dbae..d3243d93daf4 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -813,7 +813,7 @@ __used __visible void *trampoline_handler(struct pt_regs *regs)
>  			continue;
>  		/*
>  		 * Return probes must be pushed on this hash list correct
> -		 * order (same as return order) so that it can be poped
> +		 * order (same as return order) so that it can be popped
>  		 * correctly. However, if we find it is pushed it incorrect
>  		 * order, this means we find a function which should not be
>  		 * probed, because the wrong order entry is pushed on the


^ permalink raw reply

* Re: PCI: Correct the resource_alignment parameter example
From: Alexey Kardashevskiy @ 2019-06-06  4:38 UTC (permalink / raw)
  To: linux-kernel, Bjorn Helgaas; +Cc: linux-doc, linux-pci
In-Reply-To: <20190606032557.107542-1-aik@ozlabs.ru>

Argh, it should have had "PATCH" in the subject, I broke my scripts so
they run git format-patch --subject-prefix="". Sorry. Repost?


On 06/06/2019 13:25, Alexey Kardashevskiy wrote:
> The option description requires an order and so does the option
> parsing code, however the example uses a size, fix this.
> 
> Fixes: 8b078c603249 ("PCI: Update "pci=resource_alignment" documentation")
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 2b8ee90bb644..dcb53d64ad74 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3340,27 +3340,28 @@
>  		resource_alignment=
>  				Format:
>  				[<order of align>@]<pci_dev>[; ...]
>  				Specifies alignment and device to reassign
>  				aligned memory resources. How to
>  				specify the device is described above.
>  				If <order of align> is not specified,
>  				PAGE_SIZE is used as alignment.
>  				PCI-PCI bridge can be specified, if resource
>  				windows need to be expanded.
>  				To specify the alignment for several
>  				instances of a device, the PCI vendor,
>  				device, subvendor, and subdevice may be
> -				specified, e.g., 4096@pci:8086:9c22:103c:198f
> +				specified, e.g., 12@pci:8086:9c22:103c:198f
> +				for the 4096 alignment.
>  		ecrc=		Enable/disable PCIe ECRC (transaction layer
>  				end-to-end CRC checking).
>  				bios: Use BIOS/firmware settings. This is the
>  				the default.
>  				off: Turn ECRC off
>  				on: Turn ECRC on.
>  		hpiosize=nn[KMG]	The fixed amount of bus space which is
>  				reserved for hotplug bridge's IO window.
>  				Default size is 256 bytes.
>  		hpmemsize=nn[KMG]	The fixed amount of bus space which is
>  				reserved for hotplug bridge's memory window.
>  				Default size is 2 megabytes.
>  		hpbussize=nn	The minimum amount of additional bus numbers
> 

-- 
Alexey

^ permalink raw reply

* PCI: Correct the resource_alignment parameter example
From: Alexey Kardashevskiy @ 2019-06-06  3:25 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexey Kardashevskiy, linux-doc, Bjorn Helgaas, linux-pci

The option description requires an order and so does the option
parsing code, however the example uses a size, fix this.

Fixes: 8b078c603249 ("PCI: Update "pci=resource_alignment" documentation")
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 Documentation/admin-guide/kernel-parameters.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2b8ee90bb644..dcb53d64ad74 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3340,27 +3340,28 @@
 		resource_alignment=
 				Format:
 				[<order of align>@]<pci_dev>[; ...]
 				Specifies alignment and device to reassign
 				aligned memory resources. How to
 				specify the device is described above.
 				If <order of align> is not specified,
 				PAGE_SIZE is used as alignment.
 				PCI-PCI bridge can be specified, if resource
 				windows need to be expanded.
 				To specify the alignment for several
 				instances of a device, the PCI vendor,
 				device, subvendor, and subdevice may be
-				specified, e.g., 4096@pci:8086:9c22:103c:198f
+				specified, e.g., 12@pci:8086:9c22:103c:198f
+				for the 4096 alignment.
 		ecrc=		Enable/disable PCIe ECRC (transaction layer
 				end-to-end CRC checking).
 				bios: Use BIOS/firmware settings. This is the
 				the default.
 				off: Turn ECRC off
 				on: Turn ECRC on.
 		hpiosize=nn[KMG]	The fixed amount of bus space which is
 				reserved for hotplug bridge's IO window.
 				Default size is 256 bytes.
 		hpmemsize=nn[KMG]	The fixed amount of bus space which is
 				reserved for hotplug bridge's memory window.
 				Default size is 2 megabytes.
 		hpbussize=nn	The minimum amount of additional bus numbers
-- 
2.17.1


^ permalink raw reply related

* Re: [PATCH v4 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
From: Iurii Zaikin @ 2019-06-06  1:29 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
	kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
	yamada.masahiro, devicetree, dri-devel, kunit-dev, linux-doc,
	linux-fsdevel, linux-kbuild, linux-kernel, linux-kselftest,
	linux-nvdimm, linux-um, Alexander.Levin, Tim.Bird, amir73il,
	dan.carpenter, daniel, jdike, joel, julia.lawall, khilman,
	knut.omang, logang, mpe, pmladek, rdunlap, richard, rientjes,
	rostedt, wfg
In-Reply-To: <20190517182254.548EA20815@mail.kernel.org>

On Fri, May 17, 2019 at 11:22 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-05-14 15:17:10)
> > diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
> > new file mode 100644
> > index 0000000000000..fe0f2bae66085
> > --- /dev/null
> > +++ b/kernel/sysctl-test.c
> > @@ -0,0 +1,293 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit test of proc sysctl.
> > + */
> > +
> > +#include <kunit/test.h>
> > +#include <linux/printk.h>
>
> Is this include used?
  Deleted.
>
> > +#include <linux/sysctl.h>
> > +#include <linux/uaccess.h>
>
> Is this include used?
Deleted.
>
> > +
> > +
> > +static void sysctl_test_dointvec_happy_single_negative(struct kunit *test)
> > +{
> > +       struct ctl_table table = {
> > +               .procname = "foo",
> > +               .data           = &test_data.int_0001,
> > +               .maxlen         = sizeof(int),
> > +               .mode           = 0644,
> > +               .proc_handler   = proc_dointvec,
> > +               .extra1         = &i_zero,
> > +               .extra2         = &i_one_hundred,
> > +       };
> > +       char input[] = "-9";
> > +       size_t len = sizeof(input) - 1;
> > +       loff_t pos = 0;
> > +
> > +       table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> > +       KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 1, input, &len, &pos));
> > +       KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> > +       KUNIT_EXPECT_EQ(test, sizeof(input) - 1, pos);
> > +       KUNIT_EXPECT_EQ(test, -9, *(int *)table.data);
>
> Is the casting necessary? Or can the macro do a type coercion of the
> second parameter based on the first type?
 Data field is defined as void* so I believe casting is necessary to
dereference it as a pointer to an array of ints. I don't think the
macro should do any type coercion that == operator wouldn't do.
 I did change the cast to make it more clear that it's a pointer to an
array of ints being dereferenced.
>
> > +}
> > +
> > +static void sysctl_test_dointvec_single_less_int_min(struct kunit *test)
> > +{
> > +       struct ctl_table table = {
> > +               .procname = "foo",
> > +               .data           = &test_data.int_0001,
> > +               .maxlen         = sizeof(int),
> > +               .mode           = 0644,
> > +               .proc_handler   = proc_dointvec,
> > +               .extra1         = &i_zero,
> > +               .extra2         = &i_one_hundred,
> > +       };
> > +       char input[32];
> > +       size_t len = sizeof(input) - 1;
> > +       loff_t pos = 0;
> > +       unsigned long abs_of_less_than_min = (unsigned long)INT_MAX
> > +                                            - (INT_MAX + INT_MIN) + 1;
> > +
> > +       KUNIT_EXPECT_LT(test,
> > +                       snprintf(input, sizeof(input), "-%lu",
> > +                                abs_of_less_than_min),
> > +                       sizeof(input));
> > +
> > +       table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> > +       KUNIT_EXPECT_EQ(test, -EINVAL,
> > +                       proc_dointvec(&table, 1, input, &len, &pos));
> > +       KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> > +       KUNIT_EXPECT_EQ(test, 0, *(int *)table.data);
> > +}
> > +
> > +static void sysctl_test_dointvec_single_greater_int_max(struct kunit *test)
> > +{
> > +       struct ctl_table table = {
> > +               .procname = "foo",
> > +               .data           = &test_data.int_0001,
> > +               .maxlen         = sizeof(int),
> > +               .mode           = 0644,
> > +               .proc_handler   = proc_dointvec,
> > +               .extra1         = &i_zero,
> > +               .extra2         = &i_one_hundred,
> > +       };
> > +       char input[32];
> > +       size_t len = sizeof(input) - 1;
> > +       loff_t pos = 0;
> > +       unsigned long greater_than_max = (unsigned long)INT_MAX + 1;
> > +
> > +       KUNIT_EXPECT_GT(test, greater_than_max, INT_MAX);
> > +       KUNIT_EXPECT_LT(test, snprintf(input, sizeof(input), "%lu",
> > +                                      greater_than_max),
> > +                       sizeof(input));
> > +       table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> > +       KUNIT_EXPECT_EQ(test, -EINVAL,
> > +                       proc_dointvec(&table, 1, input, &len, &pos));
> > +       KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> > +       KUNIT_EXPECT_EQ(test, 0, *(int *)table.data);
> > +}
> > +
> > +static int sysctl_test_init(struct kunit *test)
> > +{
> > +       return 0;
> > +}
> > +
> > +/*
> > + * This is run once after each test case, see the comment on example_test_module
> > + * for more information.
> > + */
> > +static void sysctl_test_exit(struct kunit *test)
> > +{
> > +}
> Can the above two be omitted? If they can be empty sometimes it would be
> nice to avoid the extra symbols and code by letting them be assigned to
> NULL in the kunit_module.
 Deleted.
>
> > +
> > +/*
> > + * Here we make a list of all the test cases we want to add to the test module
> > + * below.
> > + */
> > +static struct kunit_case sysctl_test_cases[] = {
> > +       /*
> > +        * This is a helper to create a test case object from a test case
> > +        * function; its exact function is not important to understand how to
> > +        * use KUnit, just know that this is how you associate test cases with a
> > +        * test module.
> > +        */
> > +       KUNIT_CASE(sysctl_test_dointvec_null_tbl_data),
> > +       KUNIT_CASE(sysctl_test_dointvec_table_maxlen_unset),
> > +       KUNIT_CASE(sysctl_test_dointvec_table_len_is_zero),
> > +       KUNIT_CASE(sysctl_test_dointvec_table_read_but_position_set),
> > +       KUNIT_CASE(sysctl_test_dointvec_happy_single_positive),
> > +       KUNIT_CASE(sysctl_test_dointvec_happy_single_negative),
> > +       KUNIT_CASE(sysctl_test_dointvec_single_less_int_min),
> > +       KUNIT_CASE(sysctl_test_dointvec_single_greater_int_max),
> > +       {},
> > +};
> > +
> > +/*
> > + * This defines a suite or grouping of tests.
> > + *
> > + * Test cases are defined as belonging to the suite by adding them to
> > + * `test_cases`.
> > + *
> > + * Often it is desirable to run some function which will set up things which
> > + * will be used by every test; this is accomplished with an `init` function
> > + * which runs before each test case is invoked. Similarly, an `exit` function
> > + * may be specified which runs after every test case and can be used to for
> > + * cleanup. For clarity, running tests in a test module would behave as follows:
> > + *
> > + * module.init(test);
> > + * module.test_case[0](test);
> > + * module.exit(test);
> > + * module.init(test);
> > + * module.test_case[1](test);
> > + * module.exit(test);
> > + * ...;
>
> This comment (and the one above for "this is a helper") looks generic
> and should probably only be in some documentation somewhere and not for
> a sysctl test?
>
Deleted.
> > + */
> > +static struct kunit_module sysctl_test_module = {
> > +       .name = "sysctl_test",
> > +       .init = sysctl_test_init,
> > +       .exit = sysctl_test_exit,
> > +       .test_cases = sysctl_test_cases,
> > +};
> > +
> > +/*
> > + * This registers the above test module telling KUnit that this is a suite of
> > + * tests that need to be run.
> > + */
>
> Same comment about generic comment.
>
Deleted.
> > +module_test(sysctl_test_module);
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index d5a4a4036d2f8..772af4ec70111 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -1908,6 +1908,12 @@ config TEST_SYSCTL
> >
> >           If unsure, say N.
> >
> > +config SYSCTL_KUNIT_TEST
> > +       bool "KUnit test for sysctl"
>
> Why not tristate?
>
I don't believe KUnit as a module is currently supported.
> > +       depends on KUNIT
> > +       help
> > +         Enables KUnit sysctl test.
> > +

^ permalink raw reply

* Re: [PATCH v1] docs/core-api: Add string helpers API to the list
From: Mike Rapoport @ 2019-06-05 21:39 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Jonathan Corbet, linux-doc, Andrew Morton, Mike Rapoport
In-Reply-To: <20190605163944.50803-1-andriy.shevchenko@linux.intel.com>

On Wed, Jun 05, 2019 at 07:39:44PM +0300, Andy Shevchenko wrote:
> Some times string helpers are needed, but there is nothing about them
> in the generated documentation.
> 
> Fill the gap by adding a reference to string_helpers.c exported functions.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Mike Rapoport <rppt@linux.ibm.com>

> ---
>  Documentation/core-api/kernel-api.rst | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
> index a53ec2eb8176..65ae2bf1f86d 100644
> --- a/Documentation/core-api/kernel-api.rst
> +++ b/Documentation/core-api/kernel-api.rst
> @@ -33,6 +33,9 @@ String Conversions
>  .. kernel-doc:: lib/kstrtox.c
>     :export:
> 
> +.. kernel-doc:: lib/string_helpers.c
> +   :export:
> +
>  String Manipulation
>  -------------------
> 
> -- 
> 2.20.1
> 

-- 
Sincerely yours,
Mike.


^ permalink raw reply

* Re: [PATCH v1] docs/core-api: Add integer power functions to the list
From: Mike Rapoport @ 2019-06-05 21:39 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Jonathan Corbet, linux-doc, Andrew Morton, Mike Rapoport
In-Reply-To: <20190605165113.50972-1-andriy.shevchenko@linux.intel.com>

On Wed, Jun 05, 2019 at 07:51:13PM +0300, Andy Shevchenko wrote:
> Some times integer power functions, such as int_sqrt(), are needed, but
> there is nothing about them in the generated documentation.
> 
> Fill the gap by adding a reference to the corresponding exported functions.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Mike Rapoport <rppt@linux.ibm.com>

> ---
>  Documentation/core-api/kernel-api.rst | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
> index 65ae2bf1f86d..824f24ccf401 100644
> --- a/Documentation/core-api/kernel-api.rst
> +++ b/Documentation/core-api/kernel-api.rst
> @@ -141,6 +141,15 @@ Base 2 log and power Functions
>  .. kernel-doc:: include/linux/log2.h
>     :internal:
> 
> +Integer power Functions
> +-----------------------
> +
> +.. kernel-doc:: lib/math/int_pow.c
> +   :export:
> +
> +.. kernel-doc:: lib/math/int_sqrt.c
> +   :export:
> +
>  Division Functions
>  ------------------
> 
> -- 
> 2.20.1
> 

-- 
Sincerely yours,
Mike.


^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox