linux-kernel - Re: [PATCHv2 2/2] drivers/base: reorder consumer and its children behind suppliers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180625104505.GA3058@kroah.com>
Date:   Mon, 25 Jun 2018 18:45:05 +0800
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Pingfan Liu <kernelfans@...il.com>
Cc:     linux-kernel@...r.kernel.org,
        Grygorii Strashko <grygorii.strashko@...com>,
        Christoph Hellwig <hch@...radead.org>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Dave Young <dyoung@...hat.com>, linux-pci@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCHv2 2/2] drivers/base: reorder consumer and its children
 behind suppliers

On Mon, Jun 25, 2018 at 03:47:39PM +0800, Pingfan Liu wrote:
> commit 52cdbdd49853 ("driver core: correct device's shutdown order")
> introduces supplier<-consumer order in devices_kset. The commit tries
> to cleverly maintain both parent<-child and supplier<-consumer order by
> reordering a device when probing. This method makes things simple and
> clean, but unfortunately, breaks parent<-child order in some case,
> which is described in next patch in this series.

There is no "next patch in this series" :(

> Here this patch tries to resolve supplier<-consumer by only reordering a
> device when it has suppliers, and takes care of the following scenario:
>     [consumer, children] [ ... potential ... ] supplier
>                          ^                   ^
> After moving the consumer and its children after the supplier, the
> potentail section may contain consumers whose supplier is inside
> children, and this poses the requirement to dry out all consumpers in
> the section recursively.
> 
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Grygorii Strashko <grygorii.strashko@...com>
> Cc: Christoph Hellwig <hch@...radead.org>
> Cc: Bjorn Helgaas <helgaas@...nel.org>
> Cc: Dave Young <dyoung@...hat.com>
> Cc: linux-pci@...r.kernel.org
> Cc: linuxppc-dev@...ts.ozlabs.org
> Signed-off-by: Pingfan Liu <kernelfans@...il.com>
> ---
> note: there is lock issue in this patch, should be fixed in next version

Please send patches that you know are correct, why would I want to
review this if you know it is not correct?

And if the original commit is causing problems for you, why not just
revert that instead of adding this much-increased complexity?



> 
> ---
>  drivers/base/core.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 129 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 66f06ff..db30e86 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -123,12 +123,138 @@ static int device_is_dependent(struct device *dev, void *target)
>  	return ret;
>  }
>  
> -/* a temporary place holder to mark out the root cause of the bug.
> - * The proposal algorithm will come in next patch
> +struct pos_info {
> +	struct device *pos;
> +	struct device *tail;
> +};
> +
> +/* caller takes the devices_kset->list_lock */
> +static int descendants_reorder_after_pos(struct device *dev,
> +	void *data)

Why are you wrapping lines that do not need to be wrapped?

What does this function do?

> +{
> +	struct device *pos;
> +	struct pos_info *p = data;
> +
> +	pos = p->pos;
> +	pr_debug("devices_kset: Moving %s after %s\n",
> +		 dev_name(dev), dev_name(pos));

You have a device, use it for debugging, i.e. dev_dbg().

> +	device_for_each_child(dev, p, descendants_reorder_after_pos);

Recursive?

> +	/* children at the tail */
> +	list_move(&dev->kobj.entry, &pos->kobj.entry);
> +	/* record the right boundary of the section */
> +	if (p->tail == NULL)
> +		p->tail = dev;
> +	return 0;
> +}

I really do not understand what the above code is supposed to be doing :(

> +
> +/* iterate over an open section */
> +#define list_opensect_for_each_reverse(cur, left, right)	\
> +	for (cur = right->prev; cur == left; cur = cur->prev)
> +
> +static bool is_consumer(struct device *query, struct device *supplier)
> +{
> +	struct device_link *link;
> +	/* todo, lock protection */

Always run checkpatch.pl on patches so you do not get grumpy maintainers
telling you to run checkpatch.pl :(

> +	list_for_each_entry(link, &supplier->links.consumers, s_node)
> +		if (link->consumer == query)
> +			return true;
> +	return false;
> +}
> +
> +/* recursively move the potential consumers in open section (left, right)
> + * after the barrier

What barrier?

I'm stopping here as I have no idea what is going on, and this needs a
lot more work at the basic level of "it handles locking correctly"...

If you are working on this for power9, I'm guessing you work for IBM?
If so, please run this through your internal patch review process before
sending it out again...

thanks,

greg k-h