| Message ID | 1436184692-20927-4-git-send-email-eric.auger@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/07/2015 14:11, Eric Auger wrote: > diff --git a/kernel/irq/bypass.c b/kernel/irq/bypass.c > index 5d0f92b..efadbe5 100644 > --- a/kernel/irq/bypass.c > +++ b/kernel/irq/bypass.c > @@ -19,6 +19,42 @@ static LIST_HEAD(producers); > static LIST_HEAD(consumers); > static DEFINE_MUTEX(lock); > > +/* lock must be hold when calling connect */ If a lock must be held while callbacks are called, you have to document that producers and consumers must _not_ call back into the IRQ bypass manager. (If they have to, you have to document explicitly "This function can be called from producer and consumer callbacks" whenever relevant). > +static void connect(struct irq_bypass_producer *prod, > + struct irq_bypass_consumer *cons) > +{ > + if (prod->stop) > + prod->stop(prod); > + if (cons->stop) > + cons->stop(cons); > + if (prod->add_consumer) > + prod->add_consumer(prod, cons); > + if (cons->add_producer) > + cons->add_producer(cons, prod); > + if (cons->resume) > + cons->resume(cons); > + if (prod->resume) > + prod->resume(prod); > +} > + > +/* lock must be hold when calling disconnect */ > +static void disconnect(struct irq_bypass_producer *prod, > + struct irq_bypass_consumer *cons) > +{ > + if (prod->stop) > + prod->stop(prod); > + if (cons->stop) > + cons->stop(cons); > + if (cons->del_producer) > + cons->del_producer(cons, prod); > + if (prod->del_consumer) > + prod->del_consumer(prod, cons); > + if (cons->resume) > + cons->resume(cons); > + if (prod->resume) > + prod->resume(prod); > +} > +
Hi Paolo, On 07/06/2015 02:27 PM, Paolo Bonzini wrote: > > > On 06/07/2015 14:11, Eric Auger wrote: >> diff --git a/kernel/irq/bypass.c b/kernel/irq/bypass.c >> index 5d0f92b..efadbe5 100644 >> --- a/kernel/irq/bypass.c >> +++ b/kernel/irq/bypass.c >> @@ -19,6 +19,42 @@ static LIST_HEAD(producers); >> static LIST_HEAD(consumers); >> static DEFINE_MUTEX(lock); >> >> +/* lock must be hold when calling connect */ > > If a lock must be held while callbacks are called, you have to document > that producers and consumers must _not_ call back into the IRQ bypass > manager. (If they have to, you have to document explicitly "This > function can be called from producer and consumer callbacks" whenever > relevant). OK Thanks Eric > >> +static void connect(struct irq_bypass_producer *prod, >> + struct irq_bypass_consumer *cons) >> +{ >> + if (prod->stop) >> + prod->stop(prod); >> + if (cons->stop) >> + cons->stop(cons); >> + if (prod->add_consumer) >> + prod->add_consumer(prod, cons); >> + if (cons->add_producer) >> + cons->add_producer(cons, prod); >> + if (cons->resume) >> + cons->resume(cons); >> + if (prod->resume) >> + prod->resume(prod); >> +} >> + >> +/* lock must be hold when calling disconnect */ >> +static void disconnect(struct irq_bypass_producer *prod, >> + struct irq_bypass_consumer *cons) >> +{ >> + if (prod->stop) >> + prod->stop(prod); >> + if (cons->stop) >> + cons->stop(cons); >> + if (cons->del_producer) >> + cons->del_producer(cons, prod); >> + if (prod->del_consumer) >> + prod->del_consumer(prod, cons); >> + if (cons->resume) >> + cons->resume(cons); >> + if (prod->resume) >> + prod->resume(prod); >> +} >> +
On 06/07/2015 17:35, Eric Auger wrote: >>> diff --git a/kernel/irq/bypass.c b/kernel/irq/bypass.c >>> >> index 5d0f92b..efadbe5 100644 >>> >> --- a/kernel/irq/bypass.c >>> >> +++ b/kernel/irq/bypass.c >>> >> @@ -19,6 +19,42 @@ static LIST_HEAD(producers); >>> >> static LIST_HEAD(consumers); >>> >> static DEFINE_MUTEX(lock); >>> >> >>> >> +/* lock must be hold when calling connect */ >> > >> > If a lock must be held while callbacks are called, you have to document >> > that producers and consumers must _not_ call back into the IRQ bypass >> > manager. (If they have to, you have to document explicitly "This >> > function can be called from producer and consumer callbacks" whenever >> > relevant). > OK Thanks Also, please document on functions that take the irq bypass mutex that they can sleep. In fact irq_bypass_{,un}register_{producer,consumer} need kerneldoc comments. The good thing is that this helps a bit forming a lock hierarchy across the subsystems, for example irq bypass mutex outside vfio_platform_irq spinlock, because you cannot have a spinlock inside the mutex. I think that all of your six callbacks are fine. Paolo
On 07/06/2015 05:57 PM, Paolo Bonzini wrote: > > > On 06/07/2015 17:35, Eric Auger wrote: >>>> diff --git a/kernel/irq/bypass.c b/kernel/irq/bypass.c >>>>>> index 5d0f92b..efadbe5 100644 >>>>>> --- a/kernel/irq/bypass.c >>>>>> +++ b/kernel/irq/bypass.c >>>>>> @@ -19,6 +19,42 @@ static LIST_HEAD(producers); >>>>>> static LIST_HEAD(consumers); >>>>>> static DEFINE_MUTEX(lock); >>>>>> >>>>>> +/* lock must be hold when calling connect */ >>>> >>>> If a lock must be held while callbacks are called, you have to document >>>> that producers and consumers must _not_ call back into the IRQ bypass >>>> manager. (If they have to, you have to document explicitly "This >>>> function can be called from producer and consumer callbacks" whenever >>>> relevant). >> OK Thanks > > Also, please document on functions that take the irq bypass mutex that > they can sleep. In fact irq_bypass_{,un}register_{producer,consumer} > need kerneldoc comments. > > The good thing is that this helps a bit forming a lock hierarchy across > the subsystems, for example irq bypass mutex outside vfio_platform_irq > spinlock, because you cannot have a spinlock inside the mutex. I think > that all of your six callbacks are fine. arghh, no that's wrong then. I have plenty of them in the KVM/arm vgic part :-( Eric > > Paolo >
On 06/07/2015 19:09, Eric Auger wrote: >> > The good thing is that this helps a bit forming a lock hierarchy across >> > the subsystems, for example irq bypass mutex outside vfio_platform_irq >> > spinlock, because you cannot have a spinlock inside the mutex. I think >> > that all of your six callbacks are fine. > arghh, no that's wrong then. I have plenty of them in the KVM/arm vgic > part :-( I checked and it's right... /me rereads AAAARGH. You cannot have a mutex inside a spinlock. What you're doing is fine. Paolo
On 07/06/2015 07:41 PM, Paolo Bonzini wrote: > > > On 06/07/2015 19:09, Eric Auger wrote: >>>> The good thing is that this helps a bit forming a lock hierarchy across >>>> the subsystems, for example irq bypass mutex outside vfio_platform_irq >>>> spinlock, because you cannot have a spinlock inside the mutex. I think >>>> that all of your six callbacks are fine. >> arghh, no that's wrong then. I have plenty of them in the KVM/arm vgic >> part :-( > > I checked and it's right... > > /me rereads > > AAAARGH. You cannot have a mutex inside a spinlock. What you're doing > is fine. Sweated up (+ heat wave in France). Was about to read again the "concurrency and race conditions" chapter of the linux driver bible. Might be worth anyway ;-) Many thanks for the review Eric > > Paolo >
On 07/07/2015 09:10, Eric Auger wrote: > On 07/06/2015 07:41 PM, Paolo Bonzini wrote: >> >> >> On 06/07/2015 19:09, Eric Auger wrote: >>>>> The good thing is that this helps a bit forming a lock hierarchy across >>>>> the subsystems, for example irq bypass mutex outside vfio_platform_irq >>>>> spinlock, because you cannot have a spinlock inside the mutex. I think >>>>> that all of your six callbacks are fine. >>> arghh, no that's wrong then. I have plenty of them in the KVM/arm vgic >>> part :-( >> >> I checked and it's right... >> >> /me rereads >> >> AAAARGH. You cannot have a mutex inside a spinlock. What you're doing >> is fine. > > Sweated up (+ heat wave in France). Same here, I also can blame the heat wave for the mistakes. :) Paolo
> -----Original Message----- > From: Eric Auger [mailto:eric.auger@linaro.org] > Sent: Monday, July 06, 2015 8:11 PM > To: eric.auger@st.com; eric.auger@linaro.org; > linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; > kvm@vger.kernel.org; christoffer.dall@linaro.org; marc.zyngier@arm.com; > alex.williamson@redhat.com; pbonzini@redhat.com; avi.kivity@gmail.com; > mtosatti@redhat.com; Wu, Feng; joro@8bytes.org; > b.reynal@virtualopensystems.com > Cc: linux-kernel@vger.kernel.org; patches@linaro.org > Subject: [RFC v2 3/6] irq: bypass: Extend skeleton for ARM forwarding control > > - [add,del]_[consumer,producer] updated to takes both the consumer and > producer handles. This is requested to combine info from both, > typically to link the source irq owned by the producer with the gsi > owned by the consumer (forwarded IRQ setup). > - new methods are added: > x stop/resume: Those are needed for forwarding since the state change > requires to entermingle actions at consumer, producer. > x consumer update for posted interrupts > - On handshake, we now call connect, disconnect which features the more > complex sequence. > - add irq on producer side > > Signed-off-by: Eric Auger <eric.auger@linaro.org> > > --- > > v1 -> v2: > - remove vfio_device, kvm, gsi, opaque fields included in v1 except common > - all those in can be retrieved with container_of in callbacks > --- > include/linux/irqbypass.h | 19 ++++++++++++++++--- > kernel/irq/bypass.c | 44 > ++++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 56 insertions(+), 7 deletions(-) > > diff --git a/include/linux/irqbypass.h b/include/linux/irqbypass.h > index 718508e..8f62235 100644 > --- a/include/linux/irqbypass.h > +++ b/include/linux/irqbypass.h > @@ -3,17 +3,30 @@ > > #include <linux/list.h> > > +struct irq_bypass_consumer; > + > struct irq_bypass_producer { > struct list_head node; > void *token; > - /* TBD */ > + int irq; /* linux irq */ > + void (*stop)(struct irq_bypass_producer *); > + void (*resume)(struct irq_bypass_producer *); > + void (*add_consumer)(struct irq_bypass_producer *, > + struct irq_bypass_consumer *); > + void (*del_consumer)(struct irq_bypass_producer *, > + struct irq_bypass_consumer *); > }; > > struct irq_bypass_consumer { > struct list_head node; > void *token; Can we add a pointer to ' struct irq_bypass_producer ', and assign it when connecting, de-assign it when disconnecting. since in some case, I need to update IRTE from the consumer side, where I cannot get the related producer info (I need irq info) without iterating it again. Thanks, Feng > - void (*add_producer)(struct irq_bypass_producer *); > - void (*del_producer)(struct irq_bypass_producer *); > + void (*stop)(struct irq_bypass_consumer *); > + void (*resume)(struct irq_bypass_consumer *); > + void (*add_producer)(struct irq_bypass_consumer *, > + struct irq_bypass_producer *); > + void (*del_producer)(struct irq_bypass_consumer *, > + struct irq_bypass_producer *); > + void (*update)(struct irq_bypass_consumer *); > }; > > int irq_bypass_register_producer(struct irq_bypass_producer *); > diff --git a/kernel/irq/bypass.c b/kernel/irq/bypass.c > index 5d0f92b..efadbe5 100644 > --- a/kernel/irq/bypass.c > +++ b/kernel/irq/bypass.c > @@ -19,6 +19,42 @@ static LIST_HEAD(producers); > static LIST_HEAD(consumers); > static DEFINE_MUTEX(lock); > > +/* lock must be hold when calling connect */ > +static void connect(struct irq_bypass_producer *prod, > + struct irq_bypass_consumer *cons) > +{ > + if (prod->stop) > + prod->stop(prod); > + if (cons->stop) > + cons->stop(cons); > + if (prod->add_consumer) > + prod->add_consumer(prod, cons); > + if (cons->add_producer) > + cons->add_producer(cons, prod); > + if (cons->resume) > + cons->resume(cons); > + if (prod->resume) > + prod->resume(prod); > +} > + > +/* lock must be hold when calling disconnect */ > +static void disconnect(struct irq_bypass_producer *prod, > + struct irq_bypass_consumer *cons) > +{ > + if (prod->stop) > + prod->stop(prod); > + if (cons->stop) > + cons->stop(cons); > + if (cons->del_producer) > + cons->del_producer(cons, prod); > + if (prod->del_consumer) > + prod->del_consumer(prod, cons); > + if (cons->resume) > + cons->resume(cons); > + if (prod->resume) > + prod->resume(prod); > +} > + > int irq_bypass_register_producer(struct irq_bypass_producer *producer) > { > struct irq_bypass_producer *tmp; > @@ -38,7 +74,7 @@ int irq_bypass_register_producer(struct > irq_bypass_producer *producer) > > list_for_each_entry(consumer, &consumers, node) { > if (consumer->token == producer->token) { > - consumer->add_producer(producer); > + connect(producer, consumer); > break; > } > } > @@ -56,7 +92,7 @@ void irq_bypass_unregister_producer(struct > irq_bypass_producer *producer) > > list_for_each_entry(consumer, &consumers, node) { > if (consumer->token == producer->token) { > - consumer->del_producer(producer); > + disconnect(producer, consumer); > break; > } > } > @@ -86,7 +122,7 @@ int irq_bypass_register_consumer(struct > irq_bypass_consumer *consumer) > > list_for_each_entry(producer, &producers, node) { > if (producer->token == consumer->token) { > - consumer->add_producer(producer); > + connect(producer, consumer); > break; > } > } > @@ -104,7 +140,7 @@ void irq_bypass_unregister_consumer(struct > irq_bypass_consumer *consumer) > > list_for_each_entry(producer, &producers, node) { > if (producer->token == consumer->token) { > - consumer->del_producer(producer); > + disconnect(producer, consumer); > break; > } > } > -- > 1.9.1
On 07/07/2015 12:58, Wu, Feng wrote: > > >> -----Original Message----- >> From: Eric Auger [mailto:eric.auger@linaro.org] >> Sent: Monday, July 06, 2015 8:11 PM >> To: eric.auger@st.com; eric.auger@linaro.org; >> linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; >> kvm@vger.kernel.org; christoffer.dall@linaro.org; marc.zyngier@arm.com; >> alex.williamson@redhat.com; pbonzini@redhat.com; avi.kivity@gmail.com; >> mtosatti@redhat.com; Wu, Feng; joro@8bytes.org; >> b.reynal@virtualopensystems.com >> Cc: linux-kernel@vger.kernel.org; patches@linaro.org >> Subject: [RFC v2 3/6] irq: bypass: Extend skeleton for ARM forwarding control >> >> - [add,del]_[consumer,producer] updated to takes both the consumer and >> producer handles. This is requested to combine info from both, >> typically to link the source irq owned by the producer with the gsi >> owned by the consumer (forwarded IRQ setup). >> - new methods are added: >> x stop/resume: Those are needed for forwarding since the state change >> requires to entermingle actions at consumer, producer. >> x consumer update for posted interrupts >> - On handshake, we now call connect, disconnect which features the more >> complex sequence. >> - add irq on producer side >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> --- >> >> v1 -> v2: >> - remove vfio_device, kvm, gsi, opaque fields included in v1 except common >> - all those in can be retrieved with container_of in callbacks >> --- >> include/linux/irqbypass.h | 19 ++++++++++++++++--- >> kernel/irq/bypass.c | 44 >> ++++++++++++++++++++++++++++++++++++++++---- >> 2 files changed, 56 insertions(+), 7 deletions(-) >> >> diff --git a/include/linux/irqbypass.h b/include/linux/irqbypass.h >> index 718508e..8f62235 100644 >> --- a/include/linux/irqbypass.h >> +++ b/include/linux/irqbypass.h >> @@ -3,17 +3,30 @@ >> >> #include <linux/list.h> >> >> +struct irq_bypass_consumer; >> + >> struct irq_bypass_producer { >> struct list_head node; >> void *token; >> - /* TBD */ >> + int irq; /* linux irq */ >> + void (*stop)(struct irq_bypass_producer *); >> + void (*resume)(struct irq_bypass_producer *); >> + void (*add_consumer)(struct irq_bypass_producer *, >> + struct irq_bypass_consumer *); >> + void (*del_consumer)(struct irq_bypass_producer *, >> + struct irq_bypass_consumer *); >> }; >> >> struct irq_bypass_consumer { >> struct list_head node; >> void *token; > > Can we add a pointer to ' struct irq_bypass_producer ', and > assign it when connecting, de-assign it when disconnecting. > since in some case, I need to update IRTE from the consumer > side, where I cannot get the related producer info (I need irq info) > without iterating it again. You can use container_of to add it to your own struct, e.g. struct irq_bypass_consumer cons; struct irq_bypass_producer *prod; Paolo > Thanks, > Feng > >> - void (*add_producer)(struct irq_bypass_producer *); >> - void (*del_producer)(struct irq_bypass_producer *); >> + void (*stop)(struct irq_bypass_consumer *); >> + void (*resume)(struct irq_bypass_consumer *); >> + void (*add_producer)(struct irq_bypass_consumer *, >> + struct irq_bypass_producer *); >> + void (*del_producer)(struct irq_bypass_consumer *, >> + struct irq_bypass_producer *); >> + void (*update)(struct irq_bypass_consumer *); >> }; >> >> int irq_bypass_register_producer(struct irq_bypass_producer *); >> diff --git a/kernel/irq/bypass.c b/kernel/irq/bypass.c >> index 5d0f92b..efadbe5 100644 >> --- a/kernel/irq/bypass.c >> +++ b/kernel/irq/bypass.c >> @@ -19,6 +19,42 @@ static LIST_HEAD(producers); >> static LIST_HEAD(consumers); >> static DEFINE_MUTEX(lock); >> >> +/* lock must be hold when calling connect */ >> +static void connect(struct irq_bypass_producer *prod, >> + struct irq_bypass_consumer *cons) >> +{ >> + if (prod->stop) >> + prod->stop(prod); >> + if (cons->stop) >> + cons->stop(cons); >> + if (prod->add_consumer) >> + prod->add_consumer(prod, cons); >> + if (cons->add_producer) >> + cons->add_producer(cons, prod); >> + if (cons->resume) >> + cons->resume(cons); >> + if (prod->resume) >> + prod->resume(prod); >> +} >> + >> +/* lock must be hold when calling disconnect */ >> +static void disconnect(struct irq_bypass_producer *prod, >> + struct irq_bypass_consumer *cons) >> +{ >> + if (prod->stop) >> + prod->stop(prod); >> + if (cons->stop) >> + cons->stop(cons); >> + if (cons->del_producer) >> + cons->del_producer(cons, prod); >> + if (prod->del_consumer) >> + prod->del_consumer(prod, cons); >> + if (cons->resume) >> + cons->resume(cons); >> + if (prod->resume) >> + prod->resume(prod); >> +} >> + >> int irq_bypass_register_producer(struct irq_bypass_producer *producer) >> { >> struct irq_bypass_producer *tmp; >> @@ -38,7 +74,7 @@ int irq_bypass_register_producer(struct >> irq_bypass_producer *producer) >> >> list_for_each_entry(consumer, &consumers, node) { >> if (consumer->token == producer->token) { >> - consumer->add_producer(producer); >> + connect(producer, consumer); >> break; >> } >> } >> @@ -56,7 +92,7 @@ void irq_bypass_unregister_producer(struct >> irq_bypass_producer *producer) >> >> list_for_each_entry(consumer, &consumers, node) { >> if (consumer->token == producer->token) { >> - consumer->del_producer(producer); >> + disconnect(producer, consumer); >> break; >> } >> } >> @@ -86,7 +122,7 @@ int irq_bypass_register_consumer(struct >> irq_bypass_consumer *consumer) >> >> list_for_each_entry(producer, &producers, node) { >> if (producer->token == consumer->token) { >> - consumer->add_producer(producer); >> + connect(producer, consumer); >> break; >> } >> } >> @@ -104,7 +140,7 @@ void irq_bypass_unregister_consumer(struct >> irq_bypass_consumer *consumer) >> >> list_for_each_entry(producer, &producers, node) { >> if (producer->token == consumer->token) { >> - consumer->del_producer(producer); >> + disconnect(producer, consumer); >> break; >> } >> } >> -- >> 1.9.1 >
> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Tuesday, July 07, 2015 7:01 PM > To: Wu, Feng; Eric Auger; eric.auger@st.com; > linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; > kvm@vger.kernel.org; christoffer.dall@linaro.org; marc.zyngier@arm.com; > alex.williamson@redhat.com; avi.kivity@gmail.com; mtosatti@redhat.com; > joro@8bytes.org; b.reynal@virtualopensystems.com > Cc: linux-kernel@vger.kernel.org; patches@linaro.org > Subject: Re: [RFC v2 3/6] irq: bypass: Extend skeleton for ARM forwarding > control > > > > On 07/07/2015 12:58, Wu, Feng wrote: > > > > > >> -----Original Message----- > >> From: Eric Auger [mailto:eric.auger@linaro.org] > >> Sent: Monday, July 06, 2015 8:11 PM > >> To: eric.auger@st.com; eric.auger@linaro.org; > >> linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; > >> kvm@vger.kernel.org; christoffer.dall@linaro.org; marc.zyngier@arm.com; > >> alex.williamson@redhat.com; pbonzini@redhat.com; avi.kivity@gmail.com; > >> mtosatti@redhat.com; Wu, Feng; joro@8bytes.org; > >> b.reynal@virtualopensystems.com > >> Cc: linux-kernel@vger.kernel.org; patches@linaro.org > >> Subject: [RFC v2 3/6] irq: bypass: Extend skeleton for ARM forwarding > control > >> > >> - [add,del]_[consumer,producer] updated to takes both the consumer and > >> producer handles. This is requested to combine info from both, > >> typically to link the source irq owned by the producer with the gsi > >> owned by the consumer (forwarded IRQ setup). > >> - new methods are added: > >> x stop/resume: Those are needed for forwarding since the state change > >> requires to entermingle actions at consumer, producer. > >> x consumer update for posted interrupts > >> - On handshake, we now call connect, disconnect which features the more > >> complex sequence. > >> - add irq on producer side > >> > >> Signed-off-by: Eric Auger <eric.auger@linaro.org> > >> > >> --- > >> > >> v1 -> v2: > >> - remove vfio_device, kvm, gsi, opaque fields included in v1 except common > >> - all those in can be retrieved with container_of in callbacks > >> --- > >> include/linux/irqbypass.h | 19 ++++++++++++++++--- > >> kernel/irq/bypass.c | 44 > >> ++++++++++++++++++++++++++++++++++++++++---- > >> 2 files changed, 56 insertions(+), 7 deletions(-) > >> > >> diff --git a/include/linux/irqbypass.h b/include/linux/irqbypass.h > >> index 718508e..8f62235 100644 > >> --- a/include/linux/irqbypass.h > >> +++ b/include/linux/irqbypass.h > >> @@ -3,17 +3,30 @@ > >> > >> #include <linux/list.h> > >> > >> +struct irq_bypass_consumer; > >> + > >> struct irq_bypass_producer { > >> struct list_head node; > >> void *token; > >> - /* TBD */ > >> + int irq; /* linux irq */ > >> + void (*stop)(struct irq_bypass_producer *); > >> + void (*resume)(struct irq_bypass_producer *); > >> + void (*add_consumer)(struct irq_bypass_producer *, > >> + struct irq_bypass_consumer *); > >> + void (*del_consumer)(struct irq_bypass_producer *, > >> + struct irq_bypass_consumer *); > >> }; > >> > >> struct irq_bypass_consumer { > >> struct list_head node; > >> void *token; > > > > Can we add a pointer to ' struct irq_bypass_producer ', and > > assign it when connecting, de-assign it when disconnecting. > > since in some case, I need to update IRTE from the consumer > > side, where I cannot get the related producer info (I need irq info) > > without iterating it again. > > You can use container_of to add it to your own struct, e.g. > > struct irq_bypass_consumer cons; > struct irq_bypass_producer *prod; Do you mean this: struct kvm_kernel_irqfd { ...... struct irq_bypass_consumer cons; struct irq_bypass_producer *prod; }; Thanks, Feng > > Paolo > > > Thanks, > > Feng > > > >> - void (*add_producer)(struct irq_bypass_producer *); > >> - void (*del_producer)(struct irq_bypass_producer *); > >> + void (*stop)(struct irq_bypass_consumer *); > >> + void (*resume)(struct irq_bypass_consumer *); > >> + void (*add_producer)(struct irq_bypass_consumer *, > >> + struct irq_bypass_producer *); > >> + void (*del_producer)(struct irq_bypass_consumer *, > >> + struct irq_bypass_producer *); > >> + void (*update)(struct irq_bypass_consumer *); > >> }; > >> > >> int irq_bypass_register_producer(struct irq_bypass_producer *); > >> diff --git a/kernel/irq/bypass.c b/kernel/irq/bypass.c > >> index 5d0f92b..efadbe5 100644 > >> --- a/kernel/irq/bypass.c > >> +++ b/kernel/irq/bypass.c > >> @@ -19,6 +19,42 @@ static LIST_HEAD(producers); > >> static LIST_HEAD(consumers); > >> static DEFINE_MUTEX(lock); > >> > >> +/* lock must be hold when calling connect */ > >> +static void connect(struct irq_bypass_producer *prod, > >> + struct irq_bypass_consumer *cons) > >> +{ > >> + if (prod->stop) > >> + prod->stop(prod); > >> + if (cons->stop) > >> + cons->stop(cons); > >> + if (prod->add_consumer) > >> + prod->add_consumer(prod, cons); > >> + if (cons->add_producer) > >> + cons->add_producer(cons, prod); > >> + if (cons->resume) > >> + cons->resume(cons); > >> + if (prod->resume) > >> + prod->resume(prod); > >> +} > >> + > >> +/* lock must be hold when calling disconnect */ > >> +static void disconnect(struct irq_bypass_producer *prod, > >> + struct irq_bypass_consumer *cons) > >> +{ > >> + if (prod->stop) > >> + prod->stop(prod); > >> + if (cons->stop) > >> + cons->stop(cons); > >> + if (cons->del_producer) > >> + cons->del_producer(cons, prod); > >> + if (prod->del_consumer) > >> + prod->del_consumer(prod, cons); > >> + if (cons->resume) > >> + cons->resume(cons); > >> + if (prod->resume) > >> + prod->resume(prod); > >> +} > >> + > >> int irq_bypass_register_producer(struct irq_bypass_producer *producer) > >> { > >> struct irq_bypass_producer *tmp; > >> @@ -38,7 +74,7 @@ int irq_bypass_register_producer(struct > >> irq_bypass_producer *producer) > >> > >> list_for_each_entry(consumer, &consumers, node) { > >> if (consumer->token == producer->token) { > >> - consumer->add_producer(producer); > >> + connect(producer, consumer); > >> break; > >> } > >> } > >> @@ -56,7 +92,7 @@ void irq_bypass_unregister_producer(struct > >> irq_bypass_producer *producer) > >> > >> list_for_each_entry(consumer, &consumers, node) { > >> if (consumer->token == producer->token) { > >> - consumer->del_producer(producer); > >> + disconnect(producer, consumer); > >> break; > >> } > >> } > >> @@ -86,7 +122,7 @@ int irq_bypass_register_consumer(struct > >> irq_bypass_consumer *consumer) > >> > >> list_for_each_entry(producer, &producers, node) { > >> if (producer->token == consumer->token) { > >> - consumer->add_producer(producer); > >> + connect(producer, consumer); > >> break; > >> } > >> } > >> @@ -104,7 +140,7 @@ void irq_bypass_unregister_consumer(struct > >> irq_bypass_consumer *consumer) > >> > >> list_for_each_entry(producer, &producers, node) { > >> if (producer->token == consumer->token) { > >> - consumer->del_producer(producer); > >> + disconnect(producer, consumer); > >> break; > >> } > >> } > >> -- > >> 1.9.1 > >
On 07/07/2015 13:13, Wu, Feng wrote: >> > You can use container_of to add it to your own struct, e.g. >> > >> > struct irq_bypass_consumer cons; >> > struct irq_bypass_producer *prod; > Do you mean this: > > struct kvm_kernel_irqfd { > > ...... > > struct irq_bypass_consumer cons; > struct irq_bypass_producer *prod; > }; Yes. Paolo
diff --git a/include/linux/irqbypass.h b/include/linux/irqbypass.h index 718508e..8f62235 100644 --- a/include/linux/irqbypass.h +++ b/include/linux/irqbypass.h @@ -3,17 +3,30 @@ #include <linux/list.h> +struct irq_bypass_consumer; + struct irq_bypass_producer { struct list_head node; void *token; - /* TBD */ + int irq; /* linux irq */ + void (*stop)(struct irq_bypass_producer *); + void (*resume)(struct irq_bypass_producer *); + void (*add_consumer)(struct irq_bypass_producer *, + struct irq_bypass_consumer *); + void (*del_consumer)(struct irq_bypass_producer *, + struct irq_bypass_consumer *); }; struct irq_bypass_consumer { struct list_head node; void *token; - void (*add_producer)(struct irq_bypass_producer *); - void (*del_producer)(struct irq_bypass_producer *); + void (*stop)(struct irq_bypass_consumer *); + void (*resume)(struct irq_bypass_consumer *); + void (*add_producer)(struct irq_bypass_consumer *, + struct irq_bypass_producer *); + void (*del_producer)(struct irq_bypass_consumer *, + struct irq_bypass_producer *); + void (*update)(struct irq_bypass_consumer *); }; int irq_bypass_register_producer(struct irq_bypass_producer *); diff --git a/kernel/irq/bypass.c b/kernel/irq/bypass.c index 5d0f92b..efadbe5 100644 --- a/kernel/irq/bypass.c +++ b/kernel/irq/bypass.c @@ -19,6 +19,42 @@ static LIST_HEAD(producers); static LIST_HEAD(consumers); static DEFINE_MUTEX(lock); +/* lock must be hold when calling connect */ +static void connect(struct irq_bypass_producer *prod, + struct irq_bypass_consumer *cons) +{ + if (prod->stop) + prod->stop(prod); + if (cons->stop) + cons->stop(cons); + if (prod->add_consumer) + prod->add_consumer(prod, cons); + if (cons->add_producer) + cons->add_producer(cons, prod); + if (cons->resume) + cons->resume(cons); + if (prod->resume) + prod->resume(prod); +} + +/* lock must be hold when calling disconnect */ +static void disconnect(struct irq_bypass_producer *prod, + struct irq_bypass_consumer *cons) +{ + if (prod->stop) + prod->stop(prod); + if (cons->stop) + cons->stop(cons); + if (cons->del_producer) + cons->del_producer(cons, prod); + if (prod->del_consumer) + prod->del_consumer(prod, cons); + if (cons->resume) + cons->resume(cons); + if (prod->resume) + prod->resume(prod); +} + int irq_bypass_register_producer(struct irq_bypass_producer *producer) { struct irq_bypass_producer *tmp; @@ -38,7 +74,7 @@ int irq_bypass_register_producer(struct irq_bypass_producer *producer) list_for_each_entry(consumer, &consumers, node) { if (consumer->token == producer->token) { - consumer->add_producer(producer); + connect(producer, consumer); break; } } @@ -56,7 +92,7 @@ void irq_bypass_unregister_producer(struct irq_bypass_producer *producer) list_for_each_entry(consumer, &consumers, node) { if (consumer->token == producer->token) { - consumer->del_producer(producer); + disconnect(producer, consumer); break; } } @@ -86,7 +122,7 @@ int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer) list_for_each_entry(producer, &producers, node) { if (producer->token == consumer->token) { - consumer->add_producer(producer); + connect(producer, consumer); break; } } @@ -104,7 +140,7 @@ void irq_bypass_unregister_consumer(struct irq_bypass_consumer *consumer) list_for_each_entry(producer, &producers, node) { if (producer->token == consumer->token) { - consumer->del_producer(producer); + disconnect(producer, consumer); break; } }
- [add,del]_[consumer,producer] updated to takes both the consumer and producer handles. This is requested to combine info from both, typically to link the source irq owned by the producer with the gsi owned by the consumer (forwarded IRQ setup). - new methods are added: x stop/resume: Those are needed for forwarding since the state change requires to entermingle actions at consumer, producer. x consumer update for posted interrupts - On handshake, we now call connect, disconnect which features the more complex sequence. - add irq on producer side Signed-off-by: Eric Auger <eric.auger@linaro.org> --- v1 -> v2: - remove vfio_device, kvm, gsi, opaque fields included in v1 except common - all those in can be retrieved with container_of in callbacks --- include/linux/irqbypass.h | 19 ++++++++++++++++--- kernel/irq/bypass.c | 44 ++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 56 insertions(+), 7 deletions(-)