On Thu, Aug 13, 2015 at 09:34:13AM +0300, Ohad Ben-Cohen wrote: > On Wed, Jul 29, 2015 at 12:51 AM, Lina Iyer <lina.iyer@xxxxxxxxxx> wrote: > >> Let's not make this more complicated than needed, so please add the > >> hwcaps member to hwspinlock_device instead of to hwspinlock struct. We > >> could always change this later if it proves to be insufficient. > >> > > But this could yield wrong locking scenarios. If banks are allowed RAW > > capability and is not enforced on a per-lock basis, a driver may lock > > using non-raw lock using the _raw API, while another driver may > > 'acquire' the lock (since the value written to the lock would be the > > same as raw api would). That is why you should have the capability on > > hwspinlock and not on hwspinlock_device. Locks that are defined are RAW > > capable should be used as RAW only. > > > > QCOM platform hwlock #7 is unique that different CPUs trying to acquire > > the lock would write different values and hence would be fine. But, the > > same is not true for other locks in the bank. > > As far as I understand, there is nothing special about QCOM's hwlock > #7 in terms of hardware. It's exactly the same lock as all the others. > > The only difference in hwlock #7 is the way you use it, and that > sounds like a decision the driver should be able to make. It's a > policy, and I'm not sure we should put it in the DT. I'm also not sure > we need this hwlock-specific complexity in the hwspinlock framework. The issue in hardwiring this into the driver itself means forfeiting extensibility. So on one side (w/ raw support), we get the ability to deal with the lock number changing. On the other side (w/o raw), we'd have to probably tie this to chip compat to figure out which lock is the 'special' if it ever changes. > > The driver already makes a decision whether to disable the interrupts > or not and whether to save their state or not. So it can also make a > decision whether to take a sw spinlock at all or not --- if the > hardware allows it. and that if should be encoded in an accessible > vendor specific (not hwlock specific) struct, which is setup by the > underlying vendor specific hwspinlock driver (no DT involved). It's arbitrary right now. The remote processor selected a number, not the processor running Linux. > > Let's go over your aforementioned concerns: > > But this could yield wrong locking scenarios. If banks are allowed RAW > > capability and is not enforced on a per-lock basis, a driver may lock > > using non-raw lock using the _raw API > > If this is allowed by the hardware, then this is a valid scenario. > There's no such thing a non-raw lock: a lock is raw if a raw > functionality is required. > > > while another driver may > > 'acquire' the lock (since the value written to the lock would be the > > same as raw api would). > > Not sure I understand this one. If a lock has already been assigned to > a driver, it cannot be re-assigned to another driver. > -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
