Re: [PATCH 02/16] bus: mhi: core: Add support for registering MHI controllers

Greg KH
2020-01-27T07:11:32Z

On Mon, Jan 27, 2020 at 12:32:52PM +0530, Manivannan Sadhasivam wrote:
> > > > + void __iomem *regs;
> > > > + dma_addr_t iova_start;
> > > > + dma_addr_t iova_stop;
> > > > + const char *fw_image;
> > > > + const char *edl_image;
> > > > + bool fbc_download;
> > > > + size_t sbl_size;
> > > > + size_t seg_len;
> > > > + u32 max_chan;
> > > > + struct mhi_chan *mhi_chan;
> > > > + struct list_head lpm_chans;
> > > > + u32 total_ev_rings;
> > > > + u32 hw_ev_rings;
> > > > + u32 sw_ev_rings;
> > > > + u32 nr_irqs_req;
> > > > + u32 nr_irqs;
> > > > + int *irq;
> > > > +
> > > > + struct mhi_event *mhi_event;
> > > > + struct mhi_cmd *mhi_cmd;
> > > > + struct mhi_ctxt *mhi_ctxt;
> > > > +
> > > > + u32 timeout_ms;
> > > > + struct mutex pm_mutex;
> > > > + bool pre_init;
> > > > + rwlock_t pm_lock;
> > > > + u32 pm_state;
> > > > + u32 db_access;
> > > > + enum mhi_ee_type ee;
> > > > + bool wake_set;
> > > > + atomic_t dev_wake;
> > > > + atomic_t alloc_size;
> > > > + atomic_t pending_pkts;
> > >
> > > Why a bunch of atomic variables when you already have a lock?
> > >
>
> So there are multiple locks used throughout the MHI stack and each one
> servers its own purpose. For instance, pm_lock protects againt the
> concurrent access to the PM state, transition_lock protects against the
> concurrent access to the state transition list, wlock protects against
> the concurrent access to device wake state. Since there are multiple
> worker threads and each trying to update these variables, we did the
> best to protect against the race condition by having all these locks.
>
> And there are these atomic variables which are again shared with the
> threads holding the above locks, precisely with threads holding read locks.
> So it becomes convenient to just use the atomic_ APIs to update these variables.

An atomic_ api is almost as heavy as a "normal" lock, so while you might
think it is convenient, it's odd that you feel it is needed. As an
example, "wake_set" and "dev_wake" look like they are happening at the
same time, yet one is going to be held with a lock and the other one
updated without one?

Anyway, I'll leave this alone, let's see what your next round looks
like...

greg k-h