Messages in this thread Patch in this message | | | From | Jolly Shah <> | Subject | Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface | Date | Thu, 30 Jan 2020 23:59:03 +0000 |
| |
Hi Greg,
On 1/27/20, 10:28 PM, "linux-kernel-owner@vger.kernel.org on behalf of Greg KH" <linux-kernel-owner@vger.kernel.org on behalf of gregkh@linuxfoundation.org> wrote:
On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote: > > > > + ret = kstrtol(tok, 16, &value); > > > > + if (ret) { > > > > + ret = -EFAULT; > > > > + goto err; > > > > + } > > > > + > > > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload); > > > > > > This feels "tricky", if you tie this to the device you have your driver > > > bound to, will this make it easier instead of having to go through the > > > ioctl callback? > > > > > > > GGS(general global storage) registers are in PMU space and linux doesn't have access to it > > Hence ioctl is used. > > Why not just a "real" call to the driver to make this type of reading? > You don't have ioctls within the kernel for other drivers to call, > that's not needed at all. > > these registers are for users and for special needs where users wants > to retain values over resets. but as they belong to PMU address space, > these interface APIs are provided. They don’t allow access to any > other registers. That's not the issue here. The issue is you are using an "internal" ioctl, instead just make a "real" call.
Sorry I am not clear. Do you mean that we should use linux standard ioctl interface instead of internal ioctl by mentioning "real" ? > > > > +int zynqmp_pm_ggs_init(struct kobject *parent_kobj) > > > > +{ > > > > + return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]); > > > > > > You might be racing userspace here and loosing :( > > > > Prob is called before user space is notified about sysfs so racing shouldn't happen. > > "shouldn't"? How do you know this? > > Since firmware driver is always built-in (we don't provide support to > use as module), user space won't be available before prob is complete. > Correct if I am wrong. Userspace starts earlier than you think, and also, use the correct interfaces for this type of thing, that is why it is there. Don't create purposfully-incorrect code :)
Sure. We will change it.
Thanks, Jolly Shah
> > > > diff --git a/drivers/firmware/xilinx/zynqmp.c > > > b/drivers/firmware/xilinx/zynqmp.c > > > > index 75bdfaa..4c1117d 100644 > > > > --- a/drivers/firmware/xilinx/zynqmp.c > > > > +++ b/drivers/firmware/xilinx/zynqmp.c > > > > @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id) > > > > case IOCTL_GET_PLL_FRAC_MODE: > > > > case IOCTL_SET_PLL_FRAC_DATA: > > > > case IOCTL_GET_PLL_FRAC_DATA: > > > > + case IOCTL_WRITE_GGS: > > > > + case IOCTL_READ_GGS: > > > > + case IOCTL_WRITE_PGGS: > > > > + case IOCTL_READ_PGGS: > > > > > > Huh??? > > > > Sorry not sure about your concern here. These registers are in PMU space and hence > > Ioctl is needed to let linux access them. > > userspace or kernelspace? > > You seem to be mixing them both here. > > They are in Platform Management Unit register address space so it > allows only secure access. Hence for linux to access it, interface > APIs are provided. Again, that's fine, but why are you creating an "internal ioctl"? Just make a real function call.
thanks,
greg k-h
| |