On 10 November 2014 20:17, Ashwin Chaugule <ashwin.chaugule@xxxxxxxxxx> wrote: > On 10 November 2014 09:16, Jassi Brar <jaswinder.singh@xxxxxxxxxx> wrote: >> On 10 November 2014 19:23, Ashwin Chaugule <ashwin.chaugule@xxxxxxxxxx> wrote: >>> On 10 November 2014 08:39, Jassi Brar <jaswinder.singh@xxxxxxxxxx> wrote: >>>> On 10 November 2014 18:27, Ashwin Chaugule <ashwin.chaugule@xxxxxxxxxx> wrote: >>>>> On 9 November 2014 23:13, Jassi Brar <jaswinder.singh@xxxxxxxxxx> wrote: >>>>>> >>>>>>>>> based on the discussions that followed since, we decided that its best >>>>>>>>> to add a separate PCC lookup and registration API. The main reason >>>>>>>>> being, we dont have a way to list all mbox providers in ACPI in a way >>>>>>>>> that DT does. e.g. in DT, the client->dev is used to look up mbox >>>>>>>>> controllers. In ACPI, a client cant specify which mbox controllers to >>>>>>>>> associate with, if it can attach to multiple. With the PCC specific >>>>>>>>> API, if the client calls it, the controller knows where to look, >>>>>>>>> because that lookup is PCC specific. >>>>>>>>> >>>>>>>>> In your patch, the assumption that PCC is the only ACPI mbox provider, >>>>>>>>> maybe true today, but that can change anytime. >>>>>>>>> >>>>>>>> Please read my patch again, we can have ACPI as well as DT populated >>>>>>>> clients. All that you intend to do with this patch can be done there >>>>>>>> and _without_ adding new apis. >>>>>>> >>>>>>> Read it again. Not arguing that your patch wont work for DT and ACPI, >>>>>>> but your assumption that ACPI supports PCC as the only mbox >>>>>>> controller, may not hold true. The global_xlate function will work >>>>>>> fine for PCC, but may not work for other ACPI (non-DT) mbox >>>>>>> controllers. Using the signature field/index byte works only for PCC. >>>>>>> We've already been through this discussion with Mark and Arnd and we >>>>>>> came up with the PCC API. >>>>>>> >>>>>> Please read it yet again. There is no assumption that PCC is the only >>>>>> mbox in ACPI (though I think that is very likely). The function and >>>>>> its argument are both named _global_. 'Signature' is mentioned only as >>>>>> an example in case of PCC. The PCC controller driver could expect the >>>>>> global_id to be 'signature' of the subspace, similarly another non-DT >>>>>> mailbox controller driver will expect its own different 'signature' >>>>>> (say 0xdead0000 | id_16bits). In the patch I submitted we try >>>>>> .global_xlate() of all such mboxes and only one, which finds its >>>>>> id-space specified, will return a channel. >>>>>> >>>>>> Ideally, global-id space isn't very clean, but for mailbox we anyway >>>>>> have to have a direct understanding between controller and client >>>>>> drivers. So having global IDs is a great tradeoff if we avoid messing >>>>>> up the api. >>>>> >>>>> How is this different than expecting the client to pass a string name >>>>> of the mbox controller it wants? >>>>> >>>> Global-ID is ugly, string matching is uglier. String matching requires >>>> changes to client and provider structures as opposed to simple >>>> numerical comparison to find a suitable channel. >>> >>> And both have the problem that we cant guarantee uniqueness [1][2]. >>> >> How? Please give some scenario. >> > > What is there to stop two users from coming up with the same signature > (0xdeadbeef / "string") for their mbox controllers? Things can break > at run time, because with your patch, the first mbox controller with > the duplicate signature/name will return a channel. The client may be > expecting a channel from the "other" mbox. > Two channels with same signature are supposed to be _identical_ i.e, either channel could serve any client asking for a channel with that signature. So even if an "unexpected" instance of the channel is assigned, the client should still be happy. If a client differentiates between 2 instances of a channel, that's probably a sign of bad design. The knowledge behind client's preference of instance should actually lie on the provider(controller) side. I am open to some example on the contrary. > Again, the reason is that ACPI does NOT provide a way for client to mbox > mapping in a way DT does. [1] You were CC'd on that thread. > That patch was obviously wrong and outright rejected by Arnd. I didn't have to object. > This patch has been under review for ~5months now and has undergone extensive > review from Mark, Arnd and Lv. We're really going around in circles > now. > You kept me out of CC for the last 9 revisions(~5months). I am not sure if I am at fault for proposing a better solution at this stage. BTW, I think Arnd will find I have take care of most(if not all) of his concerns. -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
