On 12/15/20 5:35 PM, Hans de Goede wrote:
Hi,
On 12/3/20 10:26 PM, Maximilian Luz wrote:
Add a misc-device providing user-space access to the Surface Aggregator
EC, mainly intended for debugging, testing, and reverse-engineering.
This interface gives user-space applications the ability to send
requests to the EC and receive the corresponding responses.
The device-file is managed by a pseudo platform-device and corresponding
driver to avoid dependence on the dedicated bus, allowing it to be
loaded in a minimal configuration.
Signed-off-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>
1 review comment inline:
[...]
+static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
+{
+ struct ssam_cdev_request __user *r;
+ struct ssam_cdev_request rqst;
+ struct ssam_request spec;
+ struct ssam_response rsp;
+ const void __user *plddata;
+ void __user *rspdata;
+ int status = 0, ret = 0, tmp;
+
+ r = (struct ssam_cdev_request __user *)arg;
+ ret = copy_struct_from_user(&rqst, sizeof(rqst), r, sizeof(*r));
+ if (ret)
+ goto out;
+
+ plddata = u64_to_user_ptr(rqst.payload.data);
+ rspdata = u64_to_user_ptr(rqst.response.data);
+
+ /* Setup basic request fields. */
+ spec.target_category = rqst.target_category;
+ spec.target_id = rqst.target_id;
+ spec.command_id = rqst.command_id;
+ spec.instance_id = rqst.instance_id;
+ spec.flags = rqst.flags;
+ spec.length = rqst.payload.length;
+ spec.payload = NULL;
+
+ rsp.capacity = rqst.response.length;
+ rsp.length = 0;
+ rsp.pointer = NULL;
+
+ /* Get request payload from user-space. */
+ if (spec.length) {
+ if (!plddata) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ spec.payload = kzalloc(spec.length, GFP_KERNEL);
+ if (!spec.payload) {
+ status = -ENOMEM;
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if (copy_from_user((void *)spec.payload, plddata, spec.length)) {
+ ret = -EFAULT;
+ goto out;
+ }
+ }
+
+ /* Allocate response buffer. */
+ if (rsp.capacity) {
+ if (!rspdata) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL);
+ if (!rsp.pointer) {
+ status = -ENOMEM;
+ ret = -EFAULT;
This is weird, -EFAULT should only be used if a SEGFAULT
would have been raised if the code was running in
userspace rather then in kernelspace, IOW if userspace
has provided an invalid pointer (or a too small buffer,
causing the pointer to become invalid at some point in
the buffer).
Oh, right.
IMHO you should simply do ret = -ENOMEM here.
Yes. that looks better. I will change that as suggested.
Otherwise this looks good to me.
Thanks,
Max
