<div dir="ltr">I searched all the DIMM daemons in the org. <a href="https://github.com/openbmc/openpower-vpd-parser/search?q=DIMM">https://github.com/openbmc/openpower-vpd-parser/search?q=DIMM</a><div><br></div><div>Seems to me smbios-sdr is the only public repo. Are there any other repos that implement the DIMM interface? </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 13, 2022 at 5:49 PM Nan Zhou <<a href="mailto:nanzhou@google.com">nanzhou@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I don't think "OM is optional" is really a true statement.  It is<br>optional from a dbus perspective, true, but practically speaking all of<br>our daemons have it (just at an inconsistent location).  One of the<br>primary features of the object manager is to emit the signals for<br>managed objects, such as PropertiesChanged.  If you don't have an object<br>manager you don't get those signals.  We have to have that on Sensors,<br>at least, and I'm pretty sure everyone adds them for Inventory as well.</blockquote><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">As you're seeing, there are typically two behaviors of implementation:</blockquote><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">1. Anything using ASIO places OM at the root by default.<br>2. Anything using PDI-bindings places the OM at the lowest common part<br>   in their hierarchy.  (Something like /xyz/openbmc_project/sensors or<br>   /xyz/openbmc_project typically).</blockquote><div><br></div><div>Is this documented somewhere formally so that when people ask or question about it, we can have a pointer.</div><div><br></div><div>If not, is PDI a good place to place this requirement?</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 13, 2022 at 7:36 AM Ed Tanous <<a href="mailto:edtanous@google.com" target="_blank">edtanous@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Jul 13, 2022 at 4:24 AM Patrick Williams <<a href="mailto:patrick@stwcx.xyz" target="_blank">patrick@stwcx.xyz</a>> wrote:<br>
><br>
> On Tue, Jul 12, 2022 at 11:48:31AM -0700, Ed Tanous wrote:<br>
> > We've had a couple cases in the project where patches have needed to be<br>
> > slowed down because of inconsistencies in our use of object manager, so<br>
> > IMO, the time has come to make our usage of ObjectManager consistent,<br>
> > documented, and drop support for the (not upstream) daemons that don't<br>
> > follow the rules.  As part of this, we will port forward all the upstream<br>
> > daemons to be consistent, and do our best to avoid bugs, but this email is<br>
> > intended to inform those that have non-upstream daemons about the change so<br>
> > they can resolve their implementations.<br>
><br>
> There isn't much in the dbus spec that puts requirements on where the<br>
> object manager is, but that certainly doesn't mean we can't add our own<br>
> design requirements on top of it.  Thanks for starting this.<br>
><br>
> > The basics:<br>
> > ObjectManager DBus interface will now be required for any daemon<br>
> > implementing a Sensor.Value interface or Inventory.Item interface.<br>
> ><br>
> > Daemons producing sensors will be required to place their ObjectManager at<br>
> > /xyz/openbmc_project/sensors<br>
> > Daemons producing inventory items will be required to place their<br>
> > ObjectManager at /xyz/openbmc_project/inventory.<br>
> ><br>
> > Both of these interfaces will be required to be published before claiming a<br>
> > well known name on dbus, to allow for the possibility of caching<br>
> > implementations at some point in future.<br>
><br>
> This means we can end up having N object managers in a single daemon if<br>
> it is hosting multiple namespaces?  Why not just host it at<br>
> /xyz/openbmc_project?<br>
<br>
Because rarely is the question being asked "What are all the openbmc<br>
interfaces and values for those interfaces that this application<br>
supports".  Usually the question is more specific, like "What sensors<br>
does this application support".  If we had to get all the inventory<br>
items, nics, and other stuff at the same time that we got all the<br>
sensors, it would make the responses larger.<br>
<br>
><br>
> > This was discussed pretty heavily on discord in the #development topic, and<br>
> > some of the nitty gritty details on why this is needed is available there,<br>
> > as well as I'm happy to discuss here.  This is one of those nasty<br>
> > spaghetti-code things that we've lived with for a while;  Hopefully if we<br>
> > can get this behind us, we can get some good real world performance<br>
> > improvements.<br>
><br>
> I see the background being discussed when I read through the history on<br>
> #development, but I don't see the rationale on why this was chosen.  I see one<br>
> comment that placing the OM at / is "wrong" but I don't see any justification.<br>
> Why is ".../sensors" right but "/" or "/xyz/openbmc_project" is not?<br>
<br>
GetManagedObjects doesn't have a way to filter down further, and for<br>
daemons that might return both, it's advantageous to be able to filter<br>
down more and not return every interface in one response.<br>
<br>
><br>
> We had a good chunk of this discussion about 6 months back in<br>
> phosphor-virtual-sensors where some Redfish code was broken against that<br>
> daemon because it _was_ using "/xyz/openbmc_project/sensors" and there<br>
> was a patch to move it to "/" which ended up getting merged.<br>
<br>
Yep, this is essentially the continuation of that discussion.<br>
<br>
> Fundamentally, I think it boiled down to neither being in opposition to<br>
> the standard and there was a bunch of code that already implied "/" so<br>
> it was the simplest way forward to achieve compatibility.<br>
><br>
> ><br>
> > Thoughts?<br>
><br>
> I do think that moving the OM lower in the hierarchy is probably better<br>
> because it allows us to have parts of the hierarchy which do not emit<br>
> signals, where having it on "/" does not.  I'm just trying to understand<br>
> (and hopefully document more) the rationale on why this choice was made.<br>
<br>
Sounds good.<br>
<br>
><br>
> --<br>
> Patrick Williams<br>
</blockquote></div>
</blockquote></div>