Re: [PATCH 1/3] firmware/arm_ffa: change ffa_device_register()'s parameters and return

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Sudeep.

> >  	ffa_dev = kzalloc(sizeof(*ffa_dev), GFP_KERNEL);
> >  	if (!ffa_dev) {
> >  		ida_free(&ffa_bus_id, id);
> > -		return NULL;
> > +		return ERR_PTR(-ENOMEM);
>
> I am not keen on changing the error from NULL here. -ENOMEM has its own
> logging. ida_alloc failing is very unlikely unless you have so many
> partitions in the system.
>

Okay I'll restore.

> >  	}
> >
> >  	dev = &ffa_dev->dev;
> > @@ -210,16 +216,18 @@ struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id,
> >  	dev_set_name(&ffa_dev->dev, "arm-ffa-%d", id);
> >
> >  	ffa_dev->id = id;
> > -	ffa_dev->vm_id = vm_id;
> > +	ffa_dev->vm_id = part_info->id;
> > +	ffa_dev->properties = part_info->properties;
> >  	ffa_dev->ops = ops;
> > -	uuid_copy(&ffa_dev->uuid, uuid);
> > +	import_uuid(&uuid, (u8 *)part_info->uuid);
> > +	uuid_copy(&ffa_dev->uuid, &uuid);
> >
> >  	ret = device_register(&ffa_dev->dev);
> >  	if (ret) {
> >  		dev_err(dev, "unable to register device %s err=%d\n",
> >  			dev_name(dev), ret);
>
> This error log will give the information you are adding in driver.c, so
> it is again not needed to change that.
>
> >  		put_device(dev);
> > -		return NULL;
> > +		return ERR_PTR(ret);
> >  	}
> >
> >  	return ffa_dev;
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index b14cbdae94e8..a3a9cdec7389 100644
> > --- a/drivers/firmware/arm_ffa/driver.c
> > +++ b/drivers/firmware/arm_ffa/driver.c
> > @@ -1406,23 +1406,19 @@ static int ffa_setup_partitions(void)
> >
> >  	xa_init(&drv_info->partition_info);
> >  	for (idx = 0, tpbuf = pbuf; idx < count; idx++, tpbuf++) {
> > -		import_uuid(&uuid, (u8 *)tpbuf->uuid);
> > -
> >  		/* Note that if the UUID will be uuid_null, that will require
> >  		 * ffa_bus_notifier() to find the UUID of this partition id
> >  		 * with help of ffa_device_match_uuid(). FF-A v1.1 and above
> >  		 * provides UUID here for each partition as part of the
> >  		 * discovery API and the same is passed.
> >  		 */
> > -		ffa_dev = ffa_device_register(&uuid, tpbuf->id, &ffa_drv_ops);
> > -		if (!ffa_dev) {
> > -			pr_err("%s: failed to register partition ID 0x%x\n",
> > -			       __func__, tpbuf->id);
> > +		ffa_dev = ffa_device_register(tpbuf, &ffa_drv_ops);
> > +		if (IS_ERR_OR_NULL(ffa_dev)) {
> > +			pr_err("%s: failed to register partition ID 0x%x, error %ld\n",
> > +			       __func__, tpbuf->id, PTR_ERR(ffa_dev));
>
> Drop this additional error info you are adding as bus.c will provide it.

Okay. Thanks!