From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Stefan Berger" Subject: Re: [PATCH v3 07/11] tpm: Replace device number bitmap with IDR Date: Mon, 22 Feb 2016 20:15:38 -0500 Message-ID: <201602230116.u1N1G6RT013568@d03av05.boulder.ibm.com> References: <1455885728-10315-1-git-send-email-stefanb@linux.vnet.ibm.com><1455885728-10315-8-git-send-email-stefanb@linux.vnet.ibm.com> <20160222190629.GE22088@obsidianresearch.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4718320904641190464==" Return-path: In-Reply-To: <20160222190629.GE22088-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tpmdd-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Jason Gunthorpe Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: tpmdd-devel@lists.sourceforge.net --===============4718320904641190464== Content-Type: multipart/alternative; boundary="=_alternative 0006F57285257F62_=" --=_alternative 0006F57285257F62_= Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="US-ASCII" Jason Gunthorpe wrote on 02/22/2016=20 02:06:29 PM: > From: Jason Gunthorpe > To: Stefan Berger > Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org > Date: 02/22/2016 02:06 PM > Subject: Re: [tpmdd-devel] [PATCH v3 07/11] tpm: Replace device=20 > number bitmap with IDR >=20 > On Fri, Feb 19, 2016 at 07:42:04AM -0500, Stefan Berger wrote: > > + if (chip=5Fnum =3D=3D TPM=5FANY=5FNUM) > > + chip=5Fnext =3D 0; > > + > > + mutex=5Flock(&idr=5Flock); > > + > > + do { > > + if (chip=5Fnum =3D=3D TPM=5FANY=5FNUM) { > > + chip=5Fprev =3D chip=5Fnext; > > + chip =3D idr=5Fget=5Fnext(&dev=5Fnums=5Fidr, &chip=5Fnext); > > + } else > > + chip =3D idr=5Ffind=5Fslowpath(&dev=5Fnums=5Fidr, chip=5Fnum); > > + > > + if (chip && !tpm=5Ftry=5Fget=5Fops(chip)) > > + break; > > + } while (chip=5Fnum =3D=3D TPM=5FANY=5FNUM && chip=5Fprev !=3D chip= =5Fnext); >=20 > This while loop doesn't look very good if tpm=5Ftry=5Fget=5Fops fails? >=20 > Maybe like this? >=20 > struct tpm=5Fchip *tpm=5Fchip=5Ffind=5Fget(int chip=5Fnum) > { > struct tpm=5Fchip *res =3D NULL; >=20 > mutex=5Flock(&idr=5Flock); >=20 > if (chip=5Fnum =3D=3D TPM=5FANY=5FNUM) { > struct tpm=5Fchip *chip; >=20 > chip=5Fnum =3D 0; > do { > chip =3D idr=5Fget=5Fnext(&dev=5Fnums=5Fidr, &chip=5Fnum); > if (res && !tpm=5Ftry=5Fget=5Fops(chip)) { > res =3D chip; > break; > } > } > while (chip); > } else { > res =3D idr=5Ffind=5Fslowpath(&dev=5Fnums=5Fidr, chip=5Fnum); > if (res && tpm=5Ftry=5Fget=5Fops(chip)) > res =3D NULL; > } >=20 > mutex=5Funlock(&idr=5Flock); >=20 > return res; > } >=20 > > + if (err < 0) { > > dev=5Ferr(dev, "No available tpm device numbers\n"); >=20 > I would drop the dev=5Ferr too >=20 > > @@ -247,12 +253,6 @@ static int tpm=5Fdev=5Fadd=5Fdevice(struct tpm=5Fc= hip=20 *chip) > > static void tpm=5Fdev=5Fdel=5Fdevice(struct tpm=5Fchip *chip) > > { > > cdev=5Fdel(&chip->cdev); > > - > > - /* Make the driver uncallable. */ > > - down=5Fwrite(&chip->ops=5Fsem); > > - chip->ops =3D NULL; > > - up=5Fwrite(&chip->ops=5Fsem); > > - > > device=5Fdel(&chip->dev); >=20 > Hum.. The ordering here is very important, I guess I got it slightly > wrong as well in my patch adding ops. >=20 > Lets go for this: >=20 > - Tear down /dev/tpmX, sysfs and all other user accessible entry > points > - Do device=5Fdel (get rid of all the sysfs stuff) > - NULL the IDR (disables kAPI access and allow reuse of the ID) > - NULL the ops (flush in-progress access now that new access is=20 impossible) Why NULL the ops so late and allow access even after device=5Fdel? I'd do=20 that at the beginning. Also the IDR has a two-stages: one making the chip inaccessible on the idr : idr=5Freplace(&dev=5Fnums=5F= idr,=20 NULL, chip->dev=5Fnum) the other one by freeing the IDR for re-use : idr=5Fremove(&dev=5Fnums=5Fi= r,=20 chip->dev=5Fnum) > - put=5Fdevice to kfree (via devm) My propsal: - Tear down /dev/tpmX - NULL the IDR (disable kAPI access since chip cannot be found anymore) - NULL the ops (flush in-progress access now that new access is=20 impossible) - Tear down sysfs and all other user accessible entry points - put=5Fdevice - Free the chip's device number from the IDR (makes IDR re-usable; must=20 happen after /dev/tpmX is released to avoid clashes Stefan >=20 > Jason >=20 >=20 ---------------------------------------------------------------------------= --- > Site24x7 APM Insight: Get Deep Visibility into Application Performance > APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month > Monitor end-to-end web transactions and take corrective actions now > Troubleshoot faster and improve end-user experience. Signup Now! > http://pubads.g.doubleclick.net/gampad/clk?id=3D272487151&iu=3D/4140 > =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F= =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F > tpmdd-devel mailing list > tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel >=20 --=_alternative 0006F57285257F62_= Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset="US-ASCII" Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/22/2016 02:06:29 PM:

> From: Jason Gunthorpe <jgun= thorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> To: = Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> Date: 02/22/2016 02:06 PM
= > Subject: Re: [tpmdd-devel] [PATCH v3 07/11] tpm: Replace device
> number bitmap with IDR
>
> On Fri, Feb 19, 2016 at 07:42:04AM -0500, Stefan Berger= wrote:
> > +   if (chip=5Fnum =3D=3D TPM=5FANY=5FNUM)
>= ; > +      chip=5Fnext =3D 0;
> > +
> >= +   mutex=5Flock(&idr=5Flock);
> > +
> > + &nbs= p; do {
> > +      if (chip=5Fnum =3D=3D TPM=5FANY= =5FNUM) {
> > +         chip=5Fprev =3D chip= =5Fnext;
> > +         chip =3D idr=5Fget=5Fne= xt(&dev=5Fnums=5Fidr, &chip=5Fnext);
> > +      } else
> > +=         chip =3D idr=5Ffind=5Fslowpath(&dev=5Fnums= =5Fidr, chip=5Fnum);
> > +
> > +      if (chip &am= p;& !tpm=5Ftry=5Fget=5Fops(chip))
> > +       &= nbsp; break;
> > +   } while (chip=5Fnum =3D=3D TPM=5FANY=5FN= UM && chip=5Fprev !=3D chip=5Fnext);
>
> This while loop doesn't look very good = if tpm=5Ftry=5Fget=5Fops fails?
>
> Maybe like this?
> <= br>> struct tpm=5Fchip *tpm=5Fchip=5Ffind=5Fget(int chip=5Fnum)
> = {
>    struct tpm=5Fchip *res =3D NULL;
>
> &n= bsp;  mutex=5Flock(&idr=5Flock);
>
>    if = (chip=5Fnum =3D=3D TPM=5FANY=5FNUM) {
>       struct t= pm=5Fchip *chip;
>
>       chip=5Fnum =3D 0;>       do {
>          c= hip =3D idr=5Fget=5Fnext(&dev=5Fnums=5Fidr, &chip=5Fnum);
>          if (res &&a= mp; !tpm=5Ftry=5Fget=5Fops(chip)) {
>             res =3D chip;
> &= nbsp;           break;
>      = ;    }
>       }
>      = ; while (chip);
>    } else {
>       = res =3D idr=5Ffind=5Fslowpath(&dev=5Fnums=5Fidr, chip=5Fnum);
> &= nbsp;     if (res && tpm=5Ftry=5Fget=5Fops(chip))
>=          res =3D NULL;
>    }
= >
>    mutex=5Funlock(&idr=5Flock);
>
>= ;    return res;
> }
>
> > +   if (err= < 0) {
> >        dev=5Ferr(dev, "No = available tpm device numbers\n");
>
> I would drop the dev=5Ferr too>
> > @@ -247,12 +253,6 @@ static int tpm=5Fdev=5Fadd=5Fdevic= e(struct tpm=5Fchip *chip)
> >  static void tpm=5Fdev=5Fdel=5Fdevice(struct tpm= =5Fchip *chip)
> >  {
> >     cdev=5Fdel(&= amp;chip->cdev);
> > -
> > -   /* Make the driver= uncallable. */
> > -   down=5Fwrite(&chip->ops=5Fsem)= ;
> > -   chip->ops =3D NULL;
> > -   up=5Fw= rite(&chip->ops=5Fsem);
> > -
> >     de= vice=5Fdel(&chip->dev);
>
> Hum.. The ordering here is = very important, I guess I got it slightly
> wrong as well in my patch= adding ops.
>
> Lets go for this:
>
> - Tear dow= n /dev/tpmX, sysfs and all other user accessible entry
>   point= s
> - Do device=5Fdel (get rid of all the sysfs stuff)
> - NULL= the IDR (disables kAPI access and allow reuse of the ID)
> - NULL the ops (flush in-progress access now that new access is impossible)

Why NU= LL the ops so late and allow access even after device=5Fdel? I'd do that at the beginning.

Also the IDR has a two-stages:
&nb= sp;one making the chip inaccessible on the idr : idr=5Freplace(&dev=5Fnums=5Fidr, NULL, chip->dev=5Fnum)
 the other one by freeing the IDR for re-use : idr=5Fremove(&dev=5Fnums=5Fir, chip->dev=5Fnum)
> - put=5Fdevice to kfree (via devm)
=

My propsal:

- Tear down /dev/tpmX
- NULL the IDR (d= isable kAPI access since chip cannot be found anymore)
- NULL the ops (flush i= n-progress access now that new access is impossible)
- Tear down sys= fs and all other user accessible entry points
- put=5Fdevice
= - Free the chip's device number from the IDR (makes IDR re-usable; must happen after /dev/tpmX is released to avoid clashes

   Stefan
<= font size=3D2>
>
> Jason
>
> --------------------= ----------------------------------------------------------
> Site24x7= APM Insight: Get Deep Visibility into Application Performance
> APM = + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
> Monit= or end-to-end web transactions and take corrective actions now
> Trou= bleshoot faster and improve end-user experience. Signup Now!
> http://pubads.g.doubleclick.net/gampad/c= lk?id=3D272487151&iu=3D/4140
>= =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F= =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F
&g= t; tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
&g= t; https://lists.sourceforge.net/lists/listinfo/tp= mdd-devel
>

--=_alternative 0006F57285257F62_=-- --===============4718320904641190464== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140 --===============4718320904641190464== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ tpmdd-devel mailing list tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org https://lists.sourceforge.net/lists/listinfo/tpmdd-devel --===============4718320904641190464==--