"Gopinath, Thara" <thara@xxxxxx> writes: >>>-----Original Message----- >>>From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >>>Sent: Thursday, December 10, 2009 10:36 PM >>>To: Gopinath, Thara >>>Cc: linux-omap@xxxxxxxxxxxxxxx; Paul Walmsley >>>Subject: Re: [PATCH] OMAP3: hwmod: check for clkdomain pointer before accesing it to change the sleep >>>dependencies. >>> >>>"Gopinath, Thara" <thara@xxxxxx> writes: >>> >>>>>>-----Original Message----- >>>>>>From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >>>>>>Sent: Thursday, December 10, 2009 4:39 AM >>>>>>To: Gopinath, Thara >>>>>>Cc: linux-omap@xxxxxxxxxxxxxxx; Paul Walmsley >>>>>>Subject: Re: [PATCH] OMAP3: hwmod: check for clkdomain pointer before accesing it to change the >>>sleep >>>>>>dependencies. >>>>>> >>>>>>Thara Gopinath <thara@xxxxxx> writes: >>>>>> >>>>>>> Some clock nodes like wdt1_fck, sr1_fck, sr2_fck etc do not have a >>>>>>> clock domain associated . For such nodes accessing the clock domain pointers >>>>>>> in _add_initiator_dep and _del_initiator_dep, will lead to null pointer >>>>>>> defreferencing crash. Adding support in these functions to check for >>>>>>> existence of clkdm pointer before trying to acess it. Even if tomorrow >>>>>>> we correct all the clock nodes to have an associated clock domain, checking >>>>>>> for the existence of the pointer is a good programming practice. >>>>>>> >>>>>>> Signed-off-by: Thara Gopinath <thara@xxxxxx> >>>>>>> Cc: Paul Walmsley <paul@xxxxxxxxx> >>>>>>> --- >>>>>>> This patch depends on http://patchwork.kernel.org/patch/63383/ >>>>>> >>>>>>I think clocks without associated clockdomains should be handled with >>>>>>a flag instead of a check for a NULL pointer. >>>>>> >>>>>>Your current approach will silently fail if someone forgets to add >>>>>>a clockdomain to a clock that should have one. >>>> >>>> Are you suggesting adding a flag to the clock node in case it does >>>> not have an associated clockdomain? >>> >>>Yes, or to the hwmod. >>> >>>> I am using your tree origin/pm-wip/omap_device branch. On this I see >>>> a commit (e89a95db7095d998991c564a756a33ee0ec722c5) in which a >>>> warning is printed in omap2_init_clk_clkdm if there is no >>>> clockdomain associated with a clock node. >>> >>>This is to check for the case of a missing clkdm when there should be >>>one. You're trying to solve the problem of when there is no >>>clockdomain for a given clock. >>> >>>> My idea was this warning plus the check so as to avoid the system >>>> crash would suffice. >>> >>>I still think this should be flagged explicitly in the clock/hwmod, >>>then check using the flag instead of of using NULL pointer check. >>> >>>For readability, it makes it more clear that this is checking for some >>>exceptions and not simply doing error checking. > > Agreed. We could add a flag and base the check on the flag. But > still if some clock node does not have a clkdomain and forgets to > put this flag, the system will still crash. Yes, but with a clear message as to why. > So have have both the checks ?? I don't think so, because the check you added will allow the system to continue in a known non-working state. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
