On Thu, Mar 10, 2016 at 10:44:45AM +0200, Heikki Krogerus wrote:

On Wed, Mar 09, 2016 at 04:41:12PM +0200, Heikki Krogerus wrote:

quoted

Hi Rafael,

quoted

quoted

diff --git a/drivers/base/property.c b/drivers/base/property.c
index a163f2c..a9df21a9 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c

@@ -820,11 +820,13 @@ void device_remove_property_set(struct device *dev)
 	 * the pset. If there is no real firmware node (ACPI/DT) primary
 	 * will hold the pset.
 	 */
-	if (!is_pset_node(fwnode))
+	if (is_pset_node(fwnode))
+		dev->fwnode = NULL;

I don't really like the way you clear dev->fwnode directly here.
set_primary_fwnode(dev, NULL) would be more appropriate IMO.

Also set_secondary_fwnode(dev, NULL) need not be done in that case, because it
doesn't change anything.

Moreover, if the primary node is not pset, the secondary one should only be
cleared if it is pset.

So that would mean

	if (is_pset_node(fwnode)) {
		set_primary_fwnode(dev, NULL);

So this does create an other problem. We may now end up having ERR_PTR
in the primary fwnode which of course is not considered in the
property handling code, and would need to be fixed similarly as I
fixed the secondary fwnode checking in 1/2 of this series.

But instead of doing that, the whole logic of handing the
primary/secondary fwnodes is probable a little bit broken and should
really be replaced with something better. We can fix these issues one
by one with the code we have now, but it really looks like one fix
will always result into two new problems. Andy promised to look at it.

In the meantime, would it make sense to go ahead with my proposal to
fix the original problem? As a temporary hack?

Scratch that last question. I'll just prepare a patch with your
proposal and in the same patch change the checking of the primary
fwnode into !IS_ERR_OR_NULL(fwnode). I think it should work for now.


Thanks,

-- 
heikki