Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1f6e3432f8626d8f3d2de99e7f2f51c1fdf4e58c.camel@linux.ibm.com>
Subject: Re: [PATCH net v2 1/3] s390/ism: Fix locking for forwarding of IRQs
 and events to clients
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Alexandra Winter <wintera@linux.ibm.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Wenjia Zhang <wenjia@linux.ibm.com>,
        Jan Karcher <jaka@linux.ibm.com>,
        Stefan Raspl <raspl@linux.ibm.com>,
        "David S. Miller" <davem@davemloft.net>
Cc:     linux-s390@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Fri, 07 Jul 2023 16:08:58 +0200
In-Reply-To: <8ec43fe6-218c-189f-4a90-73e482a0c5ff@linux.ibm.com>
References: <20230707105622.3332261-1-schnelle@linux.ibm.com>
         <20230707105622.3332261-2-schnelle@linux.ibm.com>
         <8ec43fe6-218c-189f-4a90-73e482a0c5ff@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-07-07 at 15:37 +0200, Alexandra Winter wrote:
>=20
> On 07.07.23 12:56, Niklas Schnelle wrote:
> [...]
> > Instead of expanding the use of the clients_lock further add a separate
> > array in struct ism_dev which references clients subscribed to the
> > device's events and IRQs. This array is protected by ism->lock which is
> > already taken in ism_handle_irq() and can be taken outside the IRQ
> > handler when adding/removing subscribers or the accessing
>=20
> 				typo? s/the accessing/accessing the/g
>=20
> > ism->sba_client_arr[]. This also means that the clients_lock is no
> > longer taken in IRQ context.
> >=20
>=20
> [...]
>=20
> > @@ -554,6 +577,7 @@ static void ism_dev_add_work_func(struct work_struc=
t *work)
> >  						 add_work);
> > =20
> >  	client->add(client->tgt_ism);
> > +	ism_setup_forwarding(client, client->tgt_ism);
> >  	atomic_dec(&client->tgt_ism->add_dev_cnt);
> >  	wake_up(&client->tgt_ism->waitq);
> >  }
> > @@ -691,7 +715,11 @@ static void ism_dev_remove_work_func(struct work_s=
truct *work)
> >  {
> >  	struct ism_client *client =3D container_of(work, struct ism_client,
> >  						 remove_work);
> > +	unsigned long flags;
> > =20
> > +	spin_lock_irqsave(&client->tgt_ism->lock, flags);
> > +	client->tgt_ism->subs[client->id] =3D NULL;
> > +	spin_unlock_irqrestore(&client->tgt_ism->lock, flags);
> >  	client->remove(client->tgt_ism);
> >  	atomic_dec(&client->tgt_ism->free_clients_cnt);
> >  	wake_up(&client->tgt_ism->waitq);
>=20
> I am not sure I like the new split. here you fix ism_dev_add_work_func() =
and ism_dev_remove_work_func(),
> that you remove in the next patch. But looks functionally ok to me.
>=20
>=20
> Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>

Thanks for your review. Yeah it's the price we pay for working
intermediate states. I think if you hadn't already invested the time to
look at the conmbined patch it might still be easier to review the
split patches.