Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree
Hi Ben, > On May 2, 2015, at 01:57 , Benjamin Herrenschmidt > wrote: > > On Fri, 2015-05-01 at 13:46 -0500, Rob Herring wrote: >> On Fri, May 1, 2015 at 10:22 AM, Benjamin Herrenschmidt >> wrote: >>> On Fri, 2015-05-01 at 07:54 -0500, Rob Herring wrote: >>> The difference seems to be whether you allocate space or just point to the FDT for various strings/data. Is that right? > * of_fdt_add_subtree() is the introduced API to do the work. Have you looked at overlays and if so why do they not work for your purposes? Why do you need to do this with the flattened tree? >>> >>> The basic idea I asked Gavin to implement is that since the FW needs to >>> provide a bunch of DT updates to Linux at runtime in the form of new >>> nodes below an existing one, rather than doing it via some new/custom >>> format, instead, have it send a bit of FDT blob to expand under an >>> existing node. >> >> Overlay = an FDT blob to graft into a live running system. Sounds like >> the same thing. >> >>> As for the details of Gavin implementation, I haven't looked at it in >>> details yet so there might be issues there, however I don't know what >>> you mean by "overlays", any pointer ? >> >> CONFIG_OF_OVERLAY >> >> http://events.linuxfoundation.org/sites/events/files/slides/dynamic-dt-keynote-v3.pdf > > Well, that looks horrendously complicated, poorly documented and totally > unused in-tree outside of the unittest stuff, yay ! It has all sort of > "features" that I don't really care about. > If it was easy to get stuff in, it would get more of the real-use drivers in. > I still don't see what it buys me other than making my FW a lot more > complex having to generate all that additional fixup etc... crap that I > don't totally get yet. > You don’t generate any additional fixups. You just compile with the option that generates all the fixups for you. > What's wrong with just unflattening the nodes in place ? The DT comes > from the FW in the first place so all the phandles are already good in > the new added blob. Internally, the FW created new nodes in its internal > representation and flattened the subtree and sends that subtree to > Linux. > > I don't plan to play "revert" either, if you unplug, I do need to remove > what's under the slot but that's true of boot time devices, not just > "new" ones, so the overlay stuff won't do the trick and I certainly > don't want to keep track… > You get all of the corner cases handled for free. Perhaps it works for your case too. Perhaps you can educate me on what you need supported and we can make sure it’s included. > Ben. > > Regards — Pantelis ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree
Hi Ben, Sorry for taking this long to respond, but I am working on the same problem right now. I thought I might have something to show, but not yet :) My PCI overlay case is different. In my case there is no firmware and there is the blob is provided as an overlay. The idea is that for a given PCI bus, when a PCI device with a matching device id, vendor id is probed a matching overlay should be applied. The trickiness lies in the way that the way that the target is different each time and how to handle generational issues (i.e. what happens if the pci device is removed before the application of the overlay occurs, what happens when multiple applications should happen in parallel, etc.) > On May 14, 2015, at 03:54 , Benjamin Herrenschmidt > wrote: > > On Wed, 2015-05-13 at 19:18 -0500, Rob Herring wrote: > >> I haven't decided really. >> >> The main thing with the current patch is I don't really like the added >> complexity to unflatten_dt_node. It is already a fairly complex >> function. Perhaps removing of "hybrid" as discussed will help? > > I agree, we should be able to make that much simpler, I was planning on > sorting that out with Gavin. > I think using overlays should cover your case without any issues. I don’t like messing with the unflatten method TBH. >> If there are things we can do to make overlays easier to use in your >> use case, I'd like to hear ideas. I don't really buy that being more >> complex than needed is an obstacle. That is very often the case to >> have common, scale-able solutions. I want to see a simple case be >> simple to support. > > Well, it's a LOT more complex from the FW perspective for a bunch of > features we don't really need, in a way because the DT update in our > case is just purely informational to avoid keeping wrong/outdated DT > bits, it has little functional impact (it might have a bit for interrupt > routing through bridges though). > > However, I am also pursuing an approach on FW side using a generation > count in our nodes and properties which we could use to generate > arbitrary overlays if we know what generation linux has. > > There might actual be a usage scenario for a generic way for our > firwmare to convey DT updates to Linux for other reasons. > > A few things that I don't find in the overlay code (but maybe I haven't > looked at it hard enough): > > - Can it remove nodes/properties ? > Yes. > - Can it "commit" a changeset so it's permanently part of the main DT ? > We will never have a concept of "revertable" changesets, if we need a > subsequent update, we will get a new overlay from FW that will remove > what needs to be removed and add what needs to be added. > The overlay when applied is a part of the kernel DT tree. It is trivial to add a mechanism that simply commits everything and tosses away the revert information. Note that in that case you have to make provisions for the unflatten blob to not be freed or for the device tree nodes/properties to be dynamically allocated. > IE, our current mechanism without overlay is fairly simple: > > - On PCI unplug, we remove all nodes below the slot (from linux), > the FW does the equivalent internally. > If you use an overlay, you just revert it and everything would be as it was before, without anything hanging below the slot node. Note that the ‘remove all nodes below the slot’ does not work for my case. That is because there are devices being instantiated under the slot (i2c busses, i2c devices, FPGAs etc) that need to be removed from the system. > - On PCI re-plug, the FW internally builds new nodes and sends a > new subtree as an FDT that we can expand/attach. > You can easily send a DT blob containing an overlay from firmware. It can be even easy, since you might not have to recreate the full blob each time, but instead using flat device tree methods to populate the few properties that change each time. > Now we could consider that subtree as a changeset that can be undone, > but that wouldn't work for boot time. And subsequent updates wouldn't > have that concept of "undoing" anyway. > I have posted another patch that does boot-time DT quirk which are non-revertable. https://lkml.org/lkml/2015/2/18/258 > IE. conceptually, what overlays do today is quite rooted around the idea > of having a fixed "base" DT and some pre-compiled DTB overlays that > get added/removed. The design completely ignore the idea of a FW that > maintains a "live" tree which we want to keep in sync, which is what we > want to do here, or what we could do with a "live" open firmware > implementation. > > Now we might be able to reconcile them, but it feels to me that the > overlay/changeset stuff is too rooted in the first concept… > The first DT overlays use case (beaglebone capes) is what got the concept started. Right now is a generic mechanism to apply modifications to the kernel live tree, with the possibility to revert them. > Ben. > > Regards — Pantelis ___
Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree
Hi Ben,
> On May 14, 2015, at 09:46 , Benjamin Herrenschmidt
> wrote:
>
> On Thu, 2015-05-14 at 09:23 +0300, Pantelis Antoniou wrote:
>
>>> A few things that I don't find in the overlay code (but maybe I haven't
>>> looked at it hard enough):
>>>
>>> - Can it remove nodes/properties ?
>>>
>>
>> Yes.
>
> Ok, I've missed that when looking at the overlay code then, I'll have to
> give it a closer look.
>
Ok, let me be more specific. It used to be able to do it ;)
The problem was the format used (a ‘-‘ prefix to the name).
Since I didn’t have clear use for it, I was asked to drop it by Grant.
The removal capability is of-course there for the revert case.
>>> - Can it "commit" a changeset so it's permanently part of the main DT ?
>>> We will never have a concept of "revertable" changesets, if we need a
>>> subsequent update, we will get a new overlay from FW that will remove
>>> what needs to be removed and add what needs to be added.
>>>
>>
>> The overlay when applied is a part of the kernel DT tree.
>> It is trivial to add a mechanism that simply commits everything and
>> tosses away the revert information.
>>
>> Note that in that case you have to make provisions for the unflatten
>> blob to not be freed or for the device tree nodes/properties to be
>> dynamically allocated.
>
> I think it makes sense to do the dynamic thing anyway...
>
>>> IE, our current mechanism without overlay is fairly simple:
>>>
>>> - On PCI unplug, we remove all nodes below the slot (from linux),
>>> the FW does the equivalent internally.
>>>
>>
>> If you use an overlay, you just revert it and everything would
>> be as it was before, without anything hanging below the slot node.
>
> Except that doesn't work for the boot time content which we get
> from the firmware as part of the initial FDT (and we can't change that
> without breaking backward compatibility).
>
OK
>> Note that the ‘remove all nodes below the slot’ does not work for my case.
>>
>> That is because there are devices being instantiated under the slot
>> (i2c busses, i2c devices, FPGAs etc) that need to be removed from the
>> system.
>
> Right while in my case, there isn't, it's just the standard OF PCI
> representation generated by FW, the main thing is that it might have
> some enriched properties for some known cable cards of external drawers
> that are good to have.
>
I see.
>>> - On PCI re-plug, the FW internally builds new nodes and sends a
>>> new subtree as an FDT that we can expand/attach.
>>>
>>
>> You can easily send a DT blob containing an overlay from firmware.
>
> Sending one is easy. Building it is not :-)
>
Heh, true ;)
>> It can be even easy, since you might not have to recreate the full blob
>> each time, but instead using flat device tree methods to populate the
>> few properties that change each time.
>
> No, we basically have our internal tree in the firmware in a format
> similar to Linux, ie, a pointer based tree. We can "flatten" it of
> course, but generating an overlay is trickier. We can, it's just more
> work and we are running out of time (I basically have to cut that FW in
> the next few days, then we'll be stuck with whatever interfaces we
> created, I have a big of time to fix bugs after that but that's about
> it).
>
Hmm, since you just want to transmit a whole subtree things are a bit
simpler.
You don’t need any of the fixups, and your target node is known.
So your overlay is simply:
/ {
fragment@0 {
target-path = “/foo”;
__overlay__ {
/* contents of the slot */
};
};
};
I think it’s possible to just bit-mangle a blob (in pseudo code).
const u8 template_overlay_blob[] = { };
flatten_slot(slot_blob);
overlay_blob = allocate_new_blob(template_overlay_blob, slot_blob);
overlay_node = find_node(overlay_blob, “/fragment@0/__overlay__);
target_prop = find_prop(overlay_blob, “/fragment@0/target-path”);
inject_slot_blob(overlay_blob, overlay_node, slot_blob);
modify_slot_target(overlay_blob, target_prop, slot_target);
I don’t think you need to re-flatten anything, shuffling bits around with
memmove should work.
>>> Now we could consider that subtree as a changeset that can be undone,
>>> but that wouldn't work for boot time. And subsequent updates wouldn't
>>> have that concept of "undoing" anyway.
>&g
Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree
Hi Ben,
> On May 14, 2015, at 10:14 , Benjamin Herrenschmidt
> wrote:
>
> On Thu, 2015-05-14 at 10:04 +0300, Pantelis Antoniou wrote:
>
>> Hmm, since you just want to transmit a whole subtree things are a bit
>> simpler.
>>
>> You don’t need any of the fixups, and your target node is known.
>>
>> So your overlay is simply:
>>
>> / {
>> fragment@0 {
>> target-path = “/foo”;
>> __overlay__ {
>> /* contents of the slot */
>> };
>> };
>> };
>>
>> I think it’s possible to just bit-mangle a blob (in pseudo code).
>>
>> const u8 template_overlay_blob[] = { };
>>
>> flatten_slot(slot_blob);
>>
>> overlay_blob = allocate_new_blob(template_overlay_blob, slot_blob);
>>
>> overlay_node = find_node(overlay_blob, “/fragment@0/__overlay__);
>> target_prop = find_prop(overlay_blob, “/fragment@0/target-path”);
>>
>> inject_slot_blob(overlay_blob, overlay_node, slot_blob);
>> modify_slot_target(overlay_blob, target_prop, slot_target);
>>
>> I don’t think you need to re-flatten anything, shuffling bits around with
>> memmove should work.
>
> Fairly gross :-)
>
You don’t want to know how sausages are made, but they are delicious :)
> But yeah generating the overlay doesn't necessarily scare me, I can
> generate a temp tree that is the overlay in which I "copy" the subtree
> (or in my internal ptr-based representation I could have a concept of
> alias which I follow while flattening).
>
> That leaves me with these problems:
>
> - No support for removing of nodes, so that needs to be added back to
> the format and to Linux unless I continue removing by hand in the PCI
> hotplug code itself
>
What kind of nodes/properties you need to remove at _application_ time?
What you describe is inserting a bunch of properties and nodes under
a slot’s device node. Reverting the overlay removes them all just fine.
> - No support for "committing" the overlay which needs to be added as
> well.
>
That’s the easiest part.
>>>>> Now we could consider that subtree as a changeset that can be undone,
>>>>> but that wouldn't work for boot time. And subsequent updates wouldn't
>>>>> have that concept of "undoing" anyway.
>>>>>
>>>>
>>>> I have posted another patch that does boot-time DT quirk which are
>>>> non-revertable.
>>>>
>>>> https://lkml.org/lkml/2015/2/18/258
>>>
>>> Not sure how that applies in my case ... I can't change the
>>> representation of the PCI subtree, this is standard OFW representation,
>>> I can't change the FW to make it an overlay-like thing at boot time,
>>> that would break existing kernels.
>>>
>>
>> The idea is to append the ‘quirk’ to the already booting device tree blob.
>
> I know but that's not how things work for me. At boot time the FW passes
> me one tree that contains all the PCI stuff it has probed.
>
>> Another idea floating around was to simple concatenate the booting blob with
>> any overlay blobs you want applied at boot time.
>
> Sure but I don't get overlay blobs at boot time.
>
>>>>> IE. conceptually, what overlays do today is quite rooted around the idea
>>>>> of having a fixed "base" DT and some pre-compiled DTB overlays that
>>>>> get added/removed. The design completely ignore the idea of a FW that
>>>>> maintains a "live" tree which we want to keep in sync, which is what we
>>>>> want to do here, or what we could do with a "live" open firmware
>>>>> implementation.
>>>>>
>>>>> Now we might be able to reconcile them, but it feels to me that the
>>>>> overlay/changeset stuff is too rooted in the first concept…
>>>>>
>>>>
>>>> The first DT overlays use case (beaglebone capes) is what got the concept
>>>> started.
>>>>
>>>> Right now is a generic mechanism to apply modifications to the kernel
>>>> live tree, with the possibility to revert them.
>>>
>>> Yes but as I said it's not really thought in term of keeping the kernel
>>> tree in sync with an external dynamically generated tree. Maybe we can
>>> fix it, but it's more complex…
>>>
>>
>> Yes it is, unfortunately.
>
> Right. Which makes the solution of just passing my bit of tree as a blob
> which I expand in Linux where I want it rather than an overlay tempting
> if we can make Gavin patch more palatable (removing the hybrid stuff
> etc…)
> .
>
I see. Well, how about this?
Who said you have to do the whole blob dance in the firmware?
You can just as easily pass the blob as it is to the linux kernel and
the kernel there can convert it to an overlay and apply it.
> Cheers,
> Ben.
>
>>> Ben.
>>>
>>>>> Ben.
>>>>>
>>>>>
>>>>
>>>> Regards
>>>>
>>>> — Pantelis
>>>
>>>
>>
>> Regards
>>
>> — Pantelis
>
>
Regards
— Pantelis
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree
Hi Ben, > On May 14, 2015, at 10:25 , Benjamin Herrenschmidt > wrote: > > On Thu, 2015-05-14 at 10:19 +0300, Pantelis Antoniou wrote: > >> >> You don’t want to know how sausages are made, but they are delicious :) > > ... most of the time :) > >>> But yeah generating the overlay doesn't necessarily scare me, I can >>> generate a temp tree that is the overlay in which I "copy" the subtree >>> (or in my internal ptr-based representation I could have a concept of >>> alias which I follow while flattening). >>> >>> That leaves me with these problems: >>> >>> - No support for removing of nodes, so that needs to be added back to >>> the format and to Linux unless I continue removing by hand in the PCI >>> hotplug code itself >>> >> >> What kind of nodes/properties you need to remove at _application_ time? > > Well, if we stick to removing by hand in Linux for the unplug case, then > none. > OK >> What you describe is inserting a bunch of properties and nodes under >> a slot’s device node. Reverting the overlay removes them all just fine. > > Except that still doesn't work for boot time :-) > > So I would have to do a special case on unplug: > > if (slot->dt_is_overlay) /* set to false at boot */ > remove_subtree_myself(); > else > undo_overlay(slot->overlay); > OK, in that case you do require removal. But in any case it’s the ‘negative’ of an already applied one, either at boot time or not. Modifying the overlay code to apply a ‘negative’ property should do the trick. Is that correct? >>> - No support for "committing" the overlay which needs to be added as >>> well. >>> >> >> That’s the easiest part. > > Yeah, I will need to get my head around the code a bit more but it > doesn't seem too scary. > >> I see. Well, how about this? >> >> Who said you have to do the whole blob dance in the firmware? >> >> You can just as easily pass the blob as it is to the linux kernel and >> the kernel there can convert it to an overlay and apply it. > > That's not that pretty but we can do that too which solve the problem of > fixing the FW interface. > > There is however an argument to be made in having the FW be able to > generate arbitrary overlays. If we ever want to pass more "property" > updates or node updates to Linux at runtime. > > A few cases have crept up on the radar, like updating the pstate tables > or VPD informations ... > > If we go down that path, then I would implement a concept of generation > count in the firmware, so I can generate an overlay that include all the > changes since the last "generation" given to Linux. > I will probably need that generation count myself for my PCI use case. > However that requires supporting removal of nodes/properties. So I'm > tempted to keep that feature on the back burner and go with an ad-hoc > interface for PCI for now. > I see. Bonne chance :) > Ben. > > Regards — Pantelis ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree
Hi Ben, > On May 14, 2015, at 10:47 , Benjamin Herrenschmidt > wrote: > [snip] So I spend some time thinking about your use case and I think it boils down to this: I have a live tree in the firmware, I have made changes and I need to reflect those changes to the live tree in the kernel. Sounds like ‘how do I generate a patch for getting those two in sync'. No? I can see where this might be useful for others as all. I think we really need to create a liblivedt like we have libfdt since we have a number of projects going about using/manipulating DT at runtime. 1. The linux kernel, with it’s own live tree implementation. 2. The device tree compiler (it has a live tree) custom implemented. 3. Your weird and wonderful (or wacky) firmware. 4. u-boot does use DT now, but it does with libfdt. I believe this is suboptimal. 5. barebox does DT as well. Most of what we want to do with DT can be abstracted in a library I think that all of those projects can use. What are your thoughts? Regards — Pantelis ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: OF_DYNAMIC node lifecycle
Hi Grant, CCing Thomas Gleixner & Steven Rostedt, since they might have a few ideas... On Jun 18, 2014, at 11:07 PM, Grant Likely wrote: > Hi Nathan and Tyrel, > > I'm looking into lifecycle issues on nodes modified by OF_DYNAMIC, and > I'm hoping you can help me. Right now, pseries seems to be the only > user of OF_DYNAMIC, but making OF_DYNAMIC work has a huge impact on > the entire kernel because it requires all DT code to manage reference > counting with iterating over nodes. Most users simply get it wrong. > Pantelis did some investigation and found that the reference counts on > a running kernel are all over the place. I have my doubts that any > code really gets it right. > > The problem is that users need to know when it is appropriate to call > of_node_get()/of_node_put(). All list traversals that exit early need > an extra call to of_node_put(), and code that is searching for a node > in the tree and holding a reference to it needs to call of_node_get(). > In hindsight it appears that drivers just can't get the lifecycle right. So we need to simplify things. > I've got a few pseries questions: > - What are the changes being requested by pseries firmware? Is it only > CPUs and memory nodes, or does it manipulate things all over the tree? > - How frequent are the changes? How many changes would be likely over > the runtime of the system? > - Are you able to verify that removed nodes are actually able to be > freed correctly? Do you have any testcases for node removal? > > I'm thinking very seriously about changing the locking semantics of DT > code entirely so that most users never have to worry about > of_node_get/put at all. If the DT code is switched to use rcu > primitives for tree iteration (which also means making DT code use > list_head, something I'm already investigating), then instead of > trying to figure out of_node_get/put rules, callers could use > rcu_read_lock()/rcu_read_unlock() to protect the region that is > searching over nodes, and only call of_node_get() if the node pointer > is needed outside the rcu read-side lock. > > I'd really like to be rid of the node reference counting entirely, but > I can't figure out a way of doing that safely, so I'd settle for > making it a lot easier to get correct. > Since we're going about changing things, how about that devtree_lock? We're using a raw_spinlock and we're always taking the lock with interrupts disabled. If we're going to make DT changes frequently during normal runtime and not only during boot time, those are bad for any kind of real-time performance. So the question is, do we really have code that access the live tree during atomic sections? Is that something we want? Enforcing this will make our lives easier, and we'll get the change to replace that spinlock with a mutex. > Thoughts? > > g. Regards -- Pantelis ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: OF_DYNAMIC node lifecycle
Hi Grant, On Jun 23, 2014, at 5:58 PM, Grant Likely wrote: > On Thu, 19 Jun 2014 11:33:20 +0300, Pantelis Antoniou > wrote: >> Hi Grant, >> >> CCing Thomas Gleixner & Steven Rostedt, since they might have a few >> ideas... >> >> On Jun 18, 2014, at 11:07 PM, Grant Likely wrote: >> >>> Hi Nathan and Tyrel, >>> >>> I'm looking into lifecycle issues on nodes modified by OF_DYNAMIC, and >>> I'm hoping you can help me. Right now, pseries seems to be the only >>> user of OF_DYNAMIC, but making OF_DYNAMIC work has a huge impact on >>> the entire kernel because it requires all DT code to manage reference >>> counting with iterating over nodes. Most users simply get it wrong. >>> Pantelis did some investigation and found that the reference counts on >>> a running kernel are all over the place. I have my doubts that any >>> code really gets it right. >>> >>> The problem is that users need to know when it is appropriate to call >>> of_node_get()/of_node_put(). All list traversals that exit early need >>> an extra call to of_node_put(), and code that is searching for a node >>> in the tree and holding a reference to it needs to call of_node_get(). >>> >> >> In hindsight it appears that drivers just can't get the lifecycle right. >> So we need to simplify things. >> >>> I've got a few pseries questions: >>> - What are the changes being requested by pseries firmware? Is it only >>> CPUs and memory nodes, or does it manipulate things all over the tree? >>> - How frequent are the changes? How many changes would be likely over >>> the runtime of the system? >>> - Are you able to verify that removed nodes are actually able to be >>> freed correctly? Do you have any testcases for node removal? >>> >>> I'm thinking very seriously about changing the locking semantics of DT >>> code entirely so that most users never have to worry about >>> of_node_get/put at all. If the DT code is switched to use rcu >>> primitives for tree iteration (which also means making DT code use >>> list_head, something I'm already investigating), then instead of >>> trying to figure out of_node_get/put rules, callers could use >>> rcu_read_lock()/rcu_read_unlock() to protect the region that is >>> searching over nodes, and only call of_node_get() if the node pointer >>> is needed outside the rcu read-side lock. >>> >>> I'd really like to be rid of the node reference counting entirely, but >>> I can't figure out a way of doing that safely, so I'd settle for >>> making it a lot easier to get correct. >>> >> >> Since we're going about changing things, how about that devtree_lock? > > I believe rcu would pretty much eliminate the devtree_lock entirely. All > modifiers would need to grab a mutex to ensure there is only one writer > at any given time, but readers would have free reign to parse the tree > however they like. > > DT writers would have to follow some strict rules about how to handle > nodes that are removed (ie. don't modify or of_node_put() them until > after rcu is syncronized), but the number of writers is very small and > we have control of all of them. > There's one final nitpick with transactions; we might need another node/property flag marking the 'in-progress' state so that can be skipped by iterators, but in general this looks good. >> We're using a raw_spinlock and we're always taking the lock with >> interrupts disabled. >> >> If we're going to make DT changes frequently during normal runtime >> and not only during boot time, those are bad for any kind of real-time >> performance. >> >> So the question is, do we really have code that access the live tree >> during atomic sections? Is that something we want? Enforcing this >> will make our lives easier, and we'll get the change to replace >> that spinlock with a mutex. > > Yes, I believe the powerpc CPU hotplug code accesses the DT in atomic > sections. I cannot put my finger on the exact code however. Nathan might > know better. But, if I'm right, the whole problem goes away with RCU. > This is just bad. Why would you need to that? > The design with RCU is to switch struct device_node and struct property > to use list_head and/or hlist_head with the _rcu accessors. They allow > items to be removed from a list without syncronizing with readers. Right > now we have two lists that need to be modified; the allnodes list
Re: OF_DYNAMIC node lifecycle
Hi Grant, On Jun 27, 2014, at 3:32 PM, Grant Likely wrote: > On Thu, 26 Jun 2014 14:59:31 -0500, Nathan Fontenot > wrote: >> On 06/25/2014 03:22 PM, Grant Likely wrote: >>> On Tue, 24 Jun 2014 15:07:05 -0500, Nathan Fontenot >>> wrote: >>>> On 06/23/2014 09:58 AM, Grant Likely wrote: >>>>> On Thu, 19 Jun 2014 11:33:20 +0300, Pantelis Antoniou >>>>> wrote: >>>>>> Hi Grant, >>>>>> [snip] >> >> This would affect property updates. When doing a property update the >> notifier passes a pointer to a struct containing a device node >> pointer and a pointer to the new device node property. >> >> I know specifically in memory property updates we grab the current version >> of the device tree property and compare it to the 'new' version that >> was passed to us. >> >> If you want to do the DT update before calling the notifier that should be >> fine for the memory update code and would only require very minimal >> updates. > > We could change the notifier to include both the old and new values. > > I've been thinking about changing the notifier format anyway. With the > addition of bulk changes, it would be more efficient to send a single > notifier for all the changes with a link to the change set instead of > one at a time. > That one has my vote. We also need a bulk change notifier, and for device driver use, some kind of wrapper for specific node/properties. At the moment a notification is fired for any change in the tree, we might work something more fine-grained. Like 'watch this node & subnodes', or 'watch this property (or set of properties)' > g. Regards -- Pantelis ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
Hi Wolfram, > On Jan 19, 2015, at 20:55 , Wolfram Sang wrote: > > Back in the days, sysfs seemed to have refcounting issues and subsystems > needed a completion to be safe. This is not the case anymore, so I2C can > get rid of this code. There is noone else besides I2C doing something > like this currently (checked with the attached coccinelle script which > checks if a release function exists and if it contains a completion). > > I have been digging through the history of linux.git and > linux-history.git and found that e.g. w1 used to have such a mechanism > and also simply removed it later. > > Some more info from Greg Kroah-Hartman: > "Having that call "wait" for the other release call to happen is really > old, as Jean points out, from 2003. We have "fixed" sysfs since then to > detach the files from the devices easier, we used to have some nasy > reference count issues in that area." > > And some testing from Jean Delvare which matches my results: > "However I just tested unloading an i2c bus driver while its adapter's > new_device attribute was opened and rmmod returned immediately. So it > doesn't look like accessing sysfs attributes actually takes a reference > to the underlying i2c_adapter." > > Let's get rid of this code before really nobody knows/understands > anymore what this was for and if it has a subtle use. > Hehe, rather obliquely tested by me too :) Please save the reference counter hackers sanity and merge this :) > Reported-by: Pantelis Antoniou > Signed-off-by: Wolfram Sang > Cc: Greg Kroah-Hartman > Cc: Jean Delvare > Cc: Julia Lawall > — > Regards — Pantelis ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: MMIO and gcc re-ordering issue
On 28 Μαϊ 2008, at 11:36 ΠΜ, Haavard Skinnemoen wrote: Benjamin Herrenschmidt <[EMAIL PROTECTED]> wrote: I'm happy to say that __raw is purely about ordering and make them byteswap on powerpc tho (ie, make them little endian like the non- raw counterpart). That would break a lot of drivers. How many actually use __raw_ * ? I do -- in all the drivers for on-chip peripherals that are shared between AT91 ARM (LE) and AVR32 (BE). Since everything goes on inside the chip, we must use LE accesses on ARM and BE accesses on AVR32. Currently, this is the only interface I know that can do native-endian accesses, so if you take it away, I'm gonna need an alternative interface that doesn't do byteswapping. Haavard I certainly do too. Quite a lot of SoC specific drivers use __raw for accessing their on-chip peripherals. Please don't change the __raw semantics, you'll end up breaking almost everything that's BE. -- Pantelis ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name
On Wed, 2017-10-18 at 10:44 -0500, Rob Herring wrote: > On Wed, Oct 18, 2017 at 10:12 AM, Alan Tull wrote: > > On Tue, Oct 17, 2017 at 6:51 PM, Frank Rowand > > wrote: > >> On 10/17/17 14:46, Rob Herring wrote: > >>> On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull wrote: > On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring wrote: > > Hi Rob, > > > With dependencies on a statically allocated full path name converted to > > use %pOF format specifier, we can store just the basename of node, and > > the unflattening of the FDT can be simplified. > > > > This commit will affect the remaining users of full_name. After > > analyzing these users, the remaining cases should only change some print > > messages. The main users of full_name are providing a name for struct > > resource. The resource names shouldn't be important other than providing > > /proc/iomem names. > > > > We no longer distinguish between pre and post 0x10 dtb formats as either > > a full path or basename will work. However, less than 0x10 formats have > > been broken since the conversion to use libfdt (and no one has cared). > > The conversion of the unflattening code to be non-recursive also broke > > pre 0x10 formats as the populate_node function would return 0 in that > > case. > > > > Signed-off-by: Rob Herring > > --- > > v2: > > - rebase to linux-next > > > > drivers/of/fdt.c | 69 > > +--- > > 1 file changed, 11 insertions(+), 58 deletions(-) > > I've just updated to the latest next branch and am finding problems > applying overlays. Reverting this commit alleviates things. The > errors I get are: > > [ 88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0 > [ 88.513447] OF: overlay: apply failed '/__symbols__' > [ 88.518423] create_overlay: Failed to create overlay (err=-12) > >>> > >>> Frank's series with overlay updates should fix this. > >> > >> Yes, it does: > >> > >> [PATCH v3 11/12] of: overlay: remove a dependency on device node > >> full_name > > > > Thanks for the fast response. I fetched the dt/next branch to test > > this but there are sufficient changes that Pantelis' "OF: DT-Overlay > > configfs interface (v7)" is broken now. I've been adding that > > downstream since 4.4. We're using it as an interface for applying > > overlays to program FPGAs. If we fix it again, is there any chance > > that can go upstream now? > > With a drive-by posting once every few years, no. > I take offense to that. There's nothing changed in the patch for years. Reposting the same patch without changes would achieve nothing. > The issue remains that the kernel is not really setup to deal with any > random property or node to be changed at any point in run-time. I > think there needs to be some restrictions around what the overlays can > touch. We can't have it be wide open and then lock things down later > and break users. One example of what you could do is you can only add > sub-trees to whitelisted nodes. That's probably acceptable for your > usecase. > Defining what can and what cannot be changed is not as trivial as a list of white-listed nodes. In some cases there is a whole node hierarchy being inserted (like in a FPGA). In others, it's merely changing a status property to "okay" and a few device parameters. The real issue is that the kernel has no way to verify that a given device tree, either at boot time or at overlay application time, is correct. When the tree is wrong at boot-time you'll hang (if you're lucky). If the tree is wrong at run-time you'll get some into some unidentified funky state. Finally what is, and what is not 'correct' is not for the kernel to decide arbitrarily, it's a matter of policy, different for each use-case. > Rob Regards -- Pantelis
Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name
Hi Frank, > On Oct 19, 2017, at 00:46 , Frank Rowand wrote: > > On 10/18/17 11:30, Rob Herring wrote: >> On Wed, Oct 18, 2017 at 10:53 AM, Pantelis Antoniou >> wrote: >>> On Wed, 2017-10-18 at 10:44 -0500, Rob Herring wrote: >>>> On Wed, Oct 18, 2017 at 10:12 AM, Alan Tull wrote: >>>>> On Tue, Oct 17, 2017 at 6:51 PM, Frank Rowand >>>>> wrote: >>>>>> On 10/17/17 14:46, Rob Herring wrote: >>>>>>> On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull wrote: >>>>>>>> On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring wrote: >>>>>>>> >>>>>>>> Hi Rob, >>>>>>>> >>>>>>>>> With dependencies on a statically allocated full path name converted >>>>>>>>> to >>>>>>>>> use %pOF format specifier, we can store just the basename of node, and >>>>>>>>> the unflattening of the FDT can be simplified. >>>>>>>>> >>>>>>>>> This commit will affect the remaining users of full_name. After >>>>>>>>> analyzing these users, the remaining cases should only change some >>>>>>>>> print >>>>>>>>> messages. The main users of full_name are providing a name for struct >>>>>>>>> resource. The resource names shouldn't be important other than >>>>>>>>> providing >>>>>>>>> /proc/iomem names. >>>>>>>>> >>>>>>>>> We no longer distinguish between pre and post 0x10 dtb formats as >>>>>>>>> either >>>>>>>>> a full path or basename will work. However, less than 0x10 formats >>>>>>>>> have >>>>>>>>> been broken since the conversion to use libfdt (and no one has cared). >>>>>>>>> The conversion of the unflattening code to be non-recursive also broke >>>>>>>>> pre 0x10 formats as the populate_node function would return 0 in that >>>>>>>>> case. >>>>>>>>> >>>>>>>>> Signed-off-by: Rob Herring >>>>>>>>> --- >>>>>>>>> v2: >>>>>>>>> - rebase to linux-next >>>>>>>>> >>>>>>>>> drivers/of/fdt.c | 69 >>>>>>>>> +--- >>>>>>>>> 1 file changed, 11 insertions(+), 58 deletions(-) >>>>>>>> >>>>>>>> I've just updated to the latest next branch and am finding problems >>>>>>>> applying overlays. Reverting this commit alleviates things. The >>>>>>>> errors I get are: >>>>>>>> >>>>>>>> [ 88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0 >>>>>>>> [ 88.513447] OF: overlay: apply failed '/__symbols__' >>>>>>>> [ 88.518423] create_overlay: Failed to create overlay (err=-12) >>>>>>> >>>>>>> Frank's series with overlay updates should fix this. >>>>>> >>>>>> Yes, it does: >>>>>> >>>>>> [PATCH v3 11/12] of: overlay: remove a dependency on device node >>>>>> full_name >>>>> >>>>> Thanks for the fast response. I fetched the dt/next branch to test >>>>> this but there are sufficient changes that Pantelis' "OF: DT-Overlay >>>>> configfs interface (v7)" is broken now. I've been adding that >>>>> downstream since 4.4. We're using it as an interface for applying >>>>> overlays to program FPGAs. If we fix it again, is there any chance >>>>> that can go upstream now? >>>> >>>> With a drive-by posting once every few years, no. >>>> >>> >>> I take offense to that. There's nothing changed in the patch for years. >>> Reposting the same patch without changes would achieve nothing. >> >> Are you still expecting review comments on it or something? >> Furthermore, If something is posted infrequently, then I'm not >> inclined to comment or care if the next posting is going to be after I >> forget what I previously said (which is n
Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name
Hi Rob, > On Oct 18, 2017, at 21:30 , Rob Herring wrote: > > On Wed, Oct 18, 2017 at 10:53 AM, Pantelis Antoniou > wrote: >> On Wed, 2017-10-18 at 10:44 -0500, Rob Herring wrote: >>> On Wed, Oct 18, 2017 at 10:12 AM, Alan Tull wrote: >>>> On Tue, Oct 17, 2017 at 6:51 PM, Frank Rowand >>>> wrote: >>>>> On 10/17/17 14:46, Rob Herring wrote: >>>>>> On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull wrote: >>>>>>> On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring wrote: >>>>>>> >>>>>>> Hi Rob, >>>>>>> >>>>>>>> With dependencies on a statically allocated full path name converted to >>>>>>>> use %pOF format specifier, we can store just the basename of node, and >>>>>>>> the unflattening of the FDT can be simplified. >>>>>>>> >>>>>>>> This commit will affect the remaining users of full_name. After >>>>>>>> analyzing these users, the remaining cases should only change some >>>>>>>> print >>>>>>>> messages. The main users of full_name are providing a name for struct >>>>>>>> resource. The resource names shouldn't be important other than >>>>>>>> providing >>>>>>>> /proc/iomem names. >>>>>>>> >>>>>>>> We no longer distinguish between pre and post 0x10 dtb formats as >>>>>>>> either >>>>>>>> a full path or basename will work. However, less than 0x10 formats have >>>>>>>> been broken since the conversion to use libfdt (and no one has cared). >>>>>>>> The conversion of the unflattening code to be non-recursive also broke >>>>>>>> pre 0x10 formats as the populate_node function would return 0 in that >>>>>>>> case. >>>>>>>> >>>>>>>> Signed-off-by: Rob Herring >>>>>>>> --- >>>>>>>> v2: >>>>>>>> - rebase to linux-next >>>>>>>> >>>>>>>> drivers/of/fdt.c | 69 >>>>>>>> +--- >>>>>>>> 1 file changed, 11 insertions(+), 58 deletions(-) >>>>>>> >>>>>>> I've just updated to the latest next branch and am finding problems >>>>>>> applying overlays. Reverting this commit alleviates things. The >>>>>>> errors I get are: >>>>>>> >>>>>>> [ 88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0 >>>>>>> [ 88.513447] OF: overlay: apply failed '/__symbols__' >>>>>>> [ 88.518423] create_overlay: Failed to create overlay (err=-12) >>>>>> >>>>>> Frank's series with overlay updates should fix this. >>>>> >>>>> Yes, it does: >>>>> >>>>> [PATCH v3 11/12] of: overlay: remove a dependency on device node >>>>> full_name >>>> >>>> Thanks for the fast response. I fetched the dt/next branch to test >>>> this but there are sufficient changes that Pantelis' "OF: DT-Overlay >>>> configfs interface (v7)" is broken now. I've been adding that >>>> downstream since 4.4. We're using it as an interface for applying >>>> overlays to program FPGAs. If we fix it again, is there any chance >>>> that can go upstream now? >>> >>> With a drive-by posting once every few years, no. >>> >> >> I take offense to that. There's nothing changed in the patch for years. >> Reposting the same patch without changes would achieve nothing. > > Are you still expecting review comments on it or something? > Furthermore, If something is posted infrequently, then I'm not > inclined to comment or care if the next posting is going to be after I > forget what I previously said (which is not very long). > > I'm just saying, don't expect to forward port, post and it will be > accepted. Below is minimally one of the issues that needs to be > addressed. > >>> The issue remains that the kernel is not really setup to deal with any >>> random property or node to be changed at any point in run-time. I >>> think there needs to be some restrictions around
Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name
Hi Frank, > On Oct 20, 2017, at 00:46 , Frank Rowand wrote: > > On 10/19/17 13:06, Moritz Fischer wrote: > > < snip > > >> We also have plenty of code that is just not aware of overlays, and >> assumes certain parts of the tree to stay static. > > I would state that somewhat differently. :-) There is very little > code that is aware of overlays, and most code assumes the device tree > does not change after early boot. > > This is one of the areas where the creation of overlays needs to be > done with care. > Correct. But this is not breaking the kernel. In general we have the following case where we load overlays (at least well formed overlays that are not doing stupid things). 1. Activation of a new device. Usually this works since is something that’s normally done at boot. 2. Deactivation of a device. This might break because the removal paths of platform device especially are not well tested (or never executed for that matter). 3. Modification of properties in an already activated device. If the device driver has not installed a device tree modification hook (as in almost 99% of the devices) it will do absolutely nothing, since the device tree is parsed only at probe time. I can argue that for these cases we could have a catch-all hook that displays a message that nothing happened. 4. Modification of some part of the tree that’s not part of a device driver, perhaps the aliases or chosen node. This may potentially be harmful or harmless depending on what has been modified. For instance modifying an already existing alias might cause internal inconsistency about device naming, while adding a new aliases should be harmless. This is a matter of policy per board, whether to allow or not. Are there other cases that are potentially more harmful? > >> I ran into that issue when I tried to add thermal zones via an overlay, >> I've been investigating how to fix the thermal framework to work with >> overlays since then and have some partially working code. >> Currently the thermal framework parses the thermal-zones node at boot, >> and assumes this stays static. This breaks with overlays. >> >> I agree we eventually need to fix the parts that break when we use >> overlays. > Regards — Pantelis
