| Message ID | 20190405084302.5548-6-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | HDCP2.2 Phase II | expand |
On Fri, Apr 05, 2019 at 02:12:54PM +0530, Ramalingam C wrote: > Functions to create and remove the binary sysfs for class are added. > > These are getting introduced as DRM wants to create the common binary > sysfs across the drm subsystem to handle hdcp srm. Why do you need individual files? That's almost always a sign that you are going to race with userspace in a bad way. Why not just use an attribute group which provides automatic support for this? thanks, greg k-h
On 2019-04-05 at 11:23:00 +0200, Greg Kroah-Hartman wrote: > On Fri, Apr 05, 2019 at 02:12:54PM +0530, Ramalingam C wrote: > > Functions to create and remove the binary sysfs for class are added. > > > > These are getting introduced as DRM wants to create the common binary > > sysfs across the drm subsystem to handle hdcp srm. > > Why do you need individual files? That's almost always a sign that you > are going to race with userspace in a bad way. Why not just use an > attribute group which provides automatic support for this? Greg, Reason behind this move is to have a common srm entry path across all drm drivers. And the data fed into this is binary blob. So I am creating a binary sysfs "hdcp_srm" at /sys/class/drm/ Thanks, Ram. > > thanks, > > greg k-h
On Fri, Apr 05, 2019 at 04:06:22PM +0530, Ramalingam C wrote: > On 2019-04-05 at 11:23:00 +0200, Greg Kroah-Hartman wrote: > > On Fri, Apr 05, 2019 at 02:12:54PM +0530, Ramalingam C wrote: > > > Functions to create and remove the binary sysfs for class are added. > > > > > > These are getting introduced as DRM wants to create the common binary > > > sysfs across the drm subsystem to handle hdcp srm. > > > > Why do you need individual files? That's almost always a sign that you > > are going to race with userspace in a bad way. Why not just use an > > attribute group which provides automatic support for this? > Greg, > > Reason behind this move is to have a common srm entry path across all drm > drivers. And the data fed into this is binary blob. So I am creating a > binary sysfs "hdcp_srm" at /sys/class/drm/ Ah, you want to have a file in your class directory, not your class device directory. No, please do not do that. There's a reason I got rid of those same types of apis in the past. And "binary blobs" are horrid anyway, they are only to be used as a pass-through to the device itself, from the kernel, no touching the data at all. If you really need/want this, then put it in the device's directory as that is where the data is going to, not the kernel "class" code as it sure as heck better not be doing anything with it. thanks, greg k-h
On 2019-04-05 at 14:32:00 +0200, Greg Kroah-Hartman wrote: > On Fri, Apr 05, 2019 at 04:06:22PM +0530, Ramalingam C wrote: > > On 2019-04-05 at 11:23:00 +0200, Greg Kroah-Hartman wrote: > > > On Fri, Apr 05, 2019 at 02:12:54PM +0530, Ramalingam C wrote: > > > > Functions to create and remove the binary sysfs for class are added. > > > > > > > > These are getting introduced as DRM wants to create the common binary > > > > sysfs across the drm subsystem to handle hdcp srm. > > > > > > Why do you need individual files? That's almost always a sign that you > > > are going to race with userspace in a bad way. Why not just use an > > > attribute group which provides automatic support for this? > > Greg, > > > > Reason behind this move is to have a common srm entry path across all drm > > drivers. And the data fed into this is binary blob. So I am creating a > > binary sysfs "hdcp_srm" at /sys/class/drm/ > > Ah, you want to have a file in your class directory, not your class > device directory. > > No, please do not do that. There's a reason I got rid of those same > types of apis in the past. > > And "binary blobs" are horrid anyway, they are only to be used as a > pass-through to the device itself, from the kernel, no touching the data > at all. If you really need/want this, then put it in the device's > directory as that is where the data is going to, not the kernel "class" > code as it sure as heck better not be doing anything with it. Greg, But here the parsing of the received binary blob is done outside the drm device/cards. This will be generic code for all drm cardx(drivers). And this will provide the service helper functions to the drm drivers for HDCP SRM checking. So we prefer to have the binary sysfs at /sys/class/drm/ than inside the cardx folders, so that it will be generic. Could you please suggest a way to achieve that? -Ram > > thanks, > > greg k-h
On Mon, Apr 15, 2019 at 06:11:13PM +0530, Ramalingam C wrote: > On 2019-04-05 at 14:32:00 +0200, Greg Kroah-Hartman wrote: > > On Fri, Apr 05, 2019 at 04:06:22PM +0530, Ramalingam C wrote: > > > On 2019-04-05 at 11:23:00 +0200, Greg Kroah-Hartman wrote: > > > > On Fri, Apr 05, 2019 at 02:12:54PM +0530, Ramalingam C wrote: > > > > > Functions to create and remove the binary sysfs for class are added. > > > > > > > > > > These are getting introduced as DRM wants to create the common binary > > > > > sysfs across the drm subsystem to handle hdcp srm. > > > > > > > > Why do you need individual files? That's almost always a sign that you > > > > are going to race with userspace in a bad way. Why not just use an > > > > attribute group which provides automatic support for this? > > > Greg, > > > > > > Reason behind this move is to have a common srm entry path across all drm > > > drivers. And the data fed into this is binary blob. So I am creating a > > > binary sysfs "hdcp_srm" at /sys/class/drm/ > > > > Ah, you want to have a file in your class directory, not your class > > device directory. > > > > No, please do not do that. There's a reason I got rid of those same > > types of apis in the past. > > > > And "binary blobs" are horrid anyway, they are only to be used as a > > pass-through to the device itself, from the kernel, no touching the data > > at all. If you really need/want this, then put it in the device's > > directory as that is where the data is going to, not the kernel "class" > > code as it sure as heck better not be doing anything with it. > Greg, > > But here the parsing of the received binary blob is done outside the drm > device/cards. This will be generic code for all drm cardx(drivers). And > this will provide the service helper functions to the drm drivers for HDCP SRM checking. Again, the kernel is NOT to be parsing any binary data that comes through a sysfs file. If you need such a crazy thing, do it through your normal drm ioctl. > So we prefer to have the binary sysfs at /sys/class/drm/ than inside the > cardx folders, so that it will be generic. Could you please suggest a way to achieve that? What is a "cardx" driver? Why can you not do it in a device-specific location? Are suddenly _ALL_ DRM drivers going to need this information? What is the use case? Who is going to be providing this blob and where is it going? What in the kernel uses it? What on the hardware uses it? What is it actually? I need a lot more information before being able to determine what you can do here. thanks, greg k-h
On 2019-04-15 at 16:47:16 +0200, Greg Kroah-Hartman wrote: > On Mon, Apr 15, 2019 at 06:11:13PM +0530, Ramalingam C wrote: > > On 2019-04-05 at 14:32:00 +0200, Greg Kroah-Hartman wrote: > > > On Fri, Apr 05, 2019 at 04:06:22PM +0530, Ramalingam C wrote: > > > > On 2019-04-05 at 11:23:00 +0200, Greg Kroah-Hartman wrote: > > > > > On Fri, Apr 05, 2019 at 02:12:54PM +0530, Ramalingam C wrote: > > > > > > Functions to create and remove the binary sysfs for class are added. > > > > > > > > > > > > These are getting introduced as DRM wants to create the common binary > > > > > > sysfs across the drm subsystem to handle hdcp srm. > > > > > > > > > > Why do you need individual files? That's almost always a sign that you > > > > > are going to race with userspace in a bad way. Why not just use an > > > > > attribute group which provides automatic support for this? > > > > Greg, > > > > > > > > Reason behind this move is to have a common srm entry path across all drm > > > > drivers. And the data fed into this is binary blob. So I am creating a > > > > binary sysfs "hdcp_srm" at /sys/class/drm/ > > > > > > Ah, you want to have a file in your class directory, not your class > > > device directory. > > > > > > No, please do not do that. There's a reason I got rid of those same > > > types of apis in the past. > > > > > > And "binary blobs" are horrid anyway, they are only to be used as a > > > pass-through to the device itself, from the kernel, no touching the data > > > at all. If you really need/want this, then put it in the device's > > > directory as that is where the data is going to, not the kernel "class" > > > code as it sure as heck better not be doing anything with it. > > Greg, > > > > But here the parsing of the received binary blob is done outside the drm > > device/cards. This will be generic code for all drm cardx(drivers). And > > this will provide the service helper functions to the drm drivers for HDCP SRM checking. > > Again, the kernel is NOT to be parsing any binary data that comes > through a sysfs file. If you need such a crazy thing, do it through > your normal drm ioctl. > > > So we prefer to have the binary sysfs at /sys/class/drm/ than inside the > > cardx folders, so that it will be generic. Could you please suggest a way to achieve that? > > What is a "cardx" driver? Meant card0 card1 etc.. Drm drivers. > Why can you not do it in a device-specific > location? Are suddenly _ALL_ DRM drivers going to need this > information? What is the use case? Who is going to be providing this > blob and where is it going? What in the kernel uses it? What on the > hardware uses it? What is it actually? This is for HDCP usecase. List of compromised receivers' ID called system renewability Message (SRM) will be available at userspace which needs to be passed to be kernel. And this data can be parsed at kernel used across all DRM drivers to implement the HDCP authentication. Hence we want generic code to parse the SRM data and provide a helper function to all DRM drivers to validate their HDCP sink's ID. To achieve this we want to keep the sysfs and parsing logic outside the drm drivers at the class level. At present I915 will be using these implementation. in future other DRM drivers can use the same. Thanks, -Ram > > I need a lot more information before being able to determine what you > can do here. > > thanks, > > greg k-h
On Mon, Apr 15, 2019 at 10:44:12PM +0530, Ramalingam C wrote: > On 2019-04-15 at 16:47:16 +0200, Greg Kroah-Hartman wrote: > > On Mon, Apr 15, 2019 at 06:11:13PM +0530, Ramalingam C wrote: > > > On 2019-04-05 at 14:32:00 +0200, Greg Kroah-Hartman wrote: > > > > On Fri, Apr 05, 2019 at 04:06:22PM +0530, Ramalingam C wrote: > > > > > On 2019-04-05 at 11:23:00 +0200, Greg Kroah-Hartman wrote: > > > > > > On Fri, Apr 05, 2019 at 02:12:54PM +0530, Ramalingam C wrote: > > > > > > > Functions to create and remove the binary sysfs for class are added. > > > > > > > > > > > > > > These are getting introduced as DRM wants to create the common binary > > > > > > > sysfs across the drm subsystem to handle hdcp srm. > > > > > > > > > > > > Why do you need individual files? That's almost always a sign that you > > > > > > are going to race with userspace in a bad way. Why not just use an > > > > > > attribute group which provides automatic support for this? > > > > > Greg, > > > > > > > > > > Reason behind this move is to have a common srm entry path across all drm > > > > > drivers. And the data fed into this is binary blob. So I am creating a > > > > > binary sysfs "hdcp_srm" at /sys/class/drm/ > > > > > > > > Ah, you want to have a file in your class directory, not your class > > > > device directory. > > > > > > > > No, please do not do that. There's a reason I got rid of those same > > > > types of apis in the past. > > > > > > > > And "binary blobs" are horrid anyway, they are only to be used as a > > > > pass-through to the device itself, from the kernel, no touching the data > > > > at all. If you really need/want this, then put it in the device's > > > > directory as that is where the data is going to, not the kernel "class" > > > > code as it sure as heck better not be doing anything with it. > > > Greg, > > > > > > But here the parsing of the received binary blob is done outside the drm > > > device/cards. This will be generic code for all drm cardx(drivers). And > > > this will provide the service helper functions to the drm drivers for HDCP SRM checking. > > > > Again, the kernel is NOT to be parsing any binary data that comes > > through a sysfs file. If you need such a crazy thing, do it through > > your normal drm ioctl. > > > > > So we prefer to have the binary sysfs at /sys/class/drm/ than inside the > > > cardx folders, so that it will be generic. Could you please suggest a way to achieve that? > > > > What is a "cardx" driver? > Meant card0 card1 etc.. Drm drivers. > > Why can you not do it in a device-specific > > location? Are suddenly _ALL_ DRM drivers going to need this > > information? What is the use case? Who is going to be providing this > > blob and where is it going? What in the kernel uses it? What on the > > hardware uses it? What is it actually? > > This is for HDCP usecase. List of compromised receivers' ID called system > renewability Message (SRM) will be available at userspace which needs to > be passed to be kernel. And this data can be parsed at kernel used > across all DRM drivers to implement the HDCP authentication. Hence we > want generic code to parse the SRM data and provide a helper function to > all DRM drivers to validate their HDCP sink's ID. > > To achieve this we want to keep the sysfs and parsing logic outside the > drm drivers at the class level. > > At present I915 will be using these implementation. in future other DRM > drivers can use the same. Again, binary sysfs files are not for any data that the kernel has to parse at all. So do not use sysfs for this. thanks, greg k-h
On Mon, Apr 15, 2019 at 8:01 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Mon, Apr 15, 2019 at 10:44:12PM +0530, Ramalingam C wrote: > > On 2019-04-15 at 16:47:16 +0200, Greg Kroah-Hartman wrote: > > > On Mon, Apr 15, 2019 at 06:11:13PM +0530, Ramalingam C wrote: > > > > On 2019-04-05 at 14:32:00 +0200, Greg Kroah-Hartman wrote: > > > > > On Fri, Apr 05, 2019 at 04:06:22PM +0530, Ramalingam C wrote: > > > > > > On 2019-04-05 at 11:23:00 +0200, Greg Kroah-Hartman wrote: > > > > > > > On Fri, Apr 05, 2019 at 02:12:54PM +0530, Ramalingam C wrote: > > > > > > > > Functions to create and remove the binary sysfs for class are added. > > > > > > > > > > > > > > > > These are getting introduced as DRM wants to create the common binary > > > > > > > > sysfs across the drm subsystem to handle hdcp srm. > > > > > > > > > > > > > > Why do you need individual files? That's almost always a sign that you > > > > > > > are going to race with userspace in a bad way. Why not just use an > > > > > > > attribute group which provides automatic support for this? > > > > > > Greg, > > > > > > > > > > > > Reason behind this move is to have a common srm entry path across all drm > > > > > > drivers. And the data fed into this is binary blob. So I am creating a > > > > > > binary sysfs "hdcp_srm" at /sys/class/drm/ > > > > > > > > > > Ah, you want to have a file in your class directory, not your class > > > > > device directory. > > > > > > > > > > No, please do not do that. There's a reason I got rid of those same > > > > > types of apis in the past. > > > > > > > > > > And "binary blobs" are horrid anyway, they are only to be used as a > > > > > pass-through to the device itself, from the kernel, no touching the data > > > > > at all. If you really need/want this, then put it in the device's > > > > > directory as that is where the data is going to, not the kernel "class" > > > > > code as it sure as heck better not be doing anything with it. > > > > Greg, > > > > > > > > But here the parsing of the received binary blob is done outside the drm > > > > device/cards. This will be generic code for all drm cardx(drivers). And > > > > this will provide the service helper functions to the drm drivers for HDCP SRM checking. > > > > > > Again, the kernel is NOT to be parsing any binary data that comes > > > through a sysfs file. If you need such a crazy thing, do it through > > > your normal drm ioctl. > > > > > > > So we prefer to have the binary sysfs at /sys/class/drm/ than inside the > > > > cardx folders, so that it will be generic. Could you please suggest a way to achieve that? > > > > > > What is a "cardx" driver? > > Meant card0 card1 etc.. Drm drivers. > > > Why can you not do it in a device-specific > > > location? Are suddenly _ALL_ DRM drivers going to need this > > > information? What is the use case? Who is going to be providing this > > > blob and where is it going? What in the kernel uses it? What on the > > > hardware uses it? What is it actually? > > > > This is for HDCP usecase. List of compromised receivers' ID called system > > renewability Message (SRM) will be available at userspace which needs to > > be passed to be kernel. And this data can be parsed at kernel used > > across all DRM drivers to implement the HDCP authentication. Hence we > > want generic code to parse the SRM data and provide a helper function to > > all DRM drivers to validate their HDCP sink's ID. > > > > To achieve this we want to keep the sysfs and parsing logic outside the > > drm drivers at the class level. > > > > At present I915 will be using these implementation. in future other DRM > > drivers can use the same. > > Again, binary sysfs files are not for any data that the kernel has to > parse at all. So do not use sysfs for this. So what's the recommend thing then? - requrest_firmware blob could work, aside that the blob might change (the point of this to allow updates). For some hw you can even stuff the SRM into some eprom iirc, so not entirely misfit. Plus kernel parses it, so not sure request_firmware is really the right thing. - configfs? I never used that one, but the transactional config stuff this provides feels like silly amounts of overkill - procfs :-) - ioctl it definitely isn't, there's no need to open a drm device node first and require a special binary to upload the srm if cat > $kernfs/srm is all that's needed - drmfs ... that's some serious amounts of overkill, so nope. - per-device sysfs binary blob, silly because it's not per-device, but wouldn't need your ack since we could sneak this one in :-) For reasonable semantics we could just alias them all to the same storage, so that userspace can do a cat > /sys/class/drm/card0/srm instead of /sys/class/drm/srm for same effects (for multi gpu systems). That's roughly why we ended up with the class sysfs file, but that seems uncool too. -Daniel
On Mon, Apr 15, 2019 at 09:14:12PM +0200, Daniel Vetter wrote: > On Mon, Apr 15, 2019 at 8:01 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Mon, Apr 15, 2019 at 10:44:12PM +0530, Ramalingam C wrote: > > > On 2019-04-15 at 16:47:16 +0200, Greg Kroah-Hartman wrote: > > > > On Mon, Apr 15, 2019 at 06:11:13PM +0530, Ramalingam C wrote: > > > > > On 2019-04-05 at 14:32:00 +0200, Greg Kroah-Hartman wrote: > > > > > > On Fri, Apr 05, 2019 at 04:06:22PM +0530, Ramalingam C wrote: > > > > > > > On 2019-04-05 at 11:23:00 +0200, Greg Kroah-Hartman wrote: > > > > > > > > On Fri, Apr 05, 2019 at 02:12:54PM +0530, Ramalingam C wrote: > > > > > > > > > Functions to create and remove the binary sysfs for class are added. > > > > > > > > > > > > > > > > > > These are getting introduced as DRM wants to create the common binary > > > > > > > > > sysfs across the drm subsystem to handle hdcp srm. > > > > > > > > > > > > > > > > Why do you need individual files? That's almost always a sign that you > > > > > > > > are going to race with userspace in a bad way. Why not just use an > > > > > > > > attribute group which provides automatic support for this? > > > > > > > Greg, > > > > > > > > > > > > > > Reason behind this move is to have a common srm entry path across all drm > > > > > > > drivers. And the data fed into this is binary blob. So I am creating a > > > > > > > binary sysfs "hdcp_srm" at /sys/class/drm/ > > > > > > > > > > > > Ah, you want to have a file in your class directory, not your class > > > > > > device directory. > > > > > > > > > > > > No, please do not do that. There's a reason I got rid of those same > > > > > > types of apis in the past. > > > > > > > > > > > > And "binary blobs" are horrid anyway, they are only to be used as a > > > > > > pass-through to the device itself, from the kernel, no touching the data > > > > > > at all. If you really need/want this, then put it in the device's > > > > > > directory as that is where the data is going to, not the kernel "class" > > > > > > code as it sure as heck better not be doing anything with it. > > > > > Greg, > > > > > > > > > > But here the parsing of the received binary blob is done outside the drm > > > > > device/cards. This will be generic code for all drm cardx(drivers). And > > > > > this will provide the service helper functions to the drm drivers for HDCP SRM checking. > > > > > > > > Again, the kernel is NOT to be parsing any binary data that comes > > > > through a sysfs file. If you need such a crazy thing, do it through > > > > your normal drm ioctl. > > > > > > > > > So we prefer to have the binary sysfs at /sys/class/drm/ than inside the > > > > > cardx folders, so that it will be generic. Could you please suggest a way to achieve that? > > > > > > > > What is a "cardx" driver? > > > Meant card0 card1 etc.. Drm drivers. > > > > Why can you not do it in a device-specific > > > > location? Are suddenly _ALL_ DRM drivers going to need this > > > > information? What is the use case? Who is going to be providing this > > > > blob and where is it going? What in the kernel uses it? What on the > > > > hardware uses it? What is it actually? > > > > > > This is for HDCP usecase. List of compromised receivers' ID called system > > > renewability Message (SRM) will be available at userspace which needs to > > > be passed to be kernel. And this data can be parsed at kernel used > > > across all DRM drivers to implement the HDCP authentication. Hence we > > > want generic code to parse the SRM data and provide a helper function to > > > all DRM drivers to validate their HDCP sink's ID. > > > > > > To achieve this we want to keep the sysfs and parsing logic outside the > > > drm drivers at the class level. > > > > > > At present I915 will be using these implementation. in future other DRM > > > drivers can use the same. > > > > Again, binary sysfs files are not for any data that the kernel has to > > parse at all. So do not use sysfs for this. > > So what's the recommend thing then? > - requrest_firmware blob could work, aside that the blob might change > (the point of this to allow updates). For some hw you can even stuff > the SRM into some eprom iirc, so not entirely misfit. Plus kernel > parses it, so not sure request_firmware is really the right thing. I think it is, as this really looks like "firmware" to me as you are wanting to do something with the buffer and pass it on. > - configfs? I never used that one, but the transactional config stuff > this provides feels like silly amounts of overkill Maybe, but why do you want to stuf binary data in your kernel in the first place? It is configuration, right, why not have it in a format that you can easily handle it in? Or is that the binary format you have? > - procfs :-) Hah, no. > - ioctl it definitely isn't, there's no need to open a drm device node > first and require a special binary to upload the srm if cat > > $kernfs/srm is all that's needed But you want this associtated with a specific drm device, right? So what's wrong with this? > - drmfs ... that's some serious amounts of overkill, so nope. 300 lines of kernel code for a fs isn't that much :) > - per-device sysfs binary blob, silly because it's not per-device, but > wouldn't need your ack since we could sneak this one in :-) For > reasonable semantics we could just alias them all to the same storage, > so that userspace can do a cat > /sys/class/drm/card0/srm instead of > /sys/class/drm/srm for same effects (for multi gpu systems). Again, no, you do not use binary sysfs files for data you want to touch. Yes, you can try to get away with it, but please do not. thanks, greg k-h
On Tue, Apr 16, 2019 at 11:04 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Mon, Apr 15, 2019 at 09:14:12PM +0200, Daniel Vetter wrote: > > On Mon, Apr 15, 2019 at 8:01 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > On Mon, Apr 15, 2019 at 10:44:12PM +0530, Ramalingam C wrote: > > > > On 2019-04-15 at 16:47:16 +0200, Greg Kroah-Hartman wrote: > > > > > On Mon, Apr 15, 2019 at 06:11:13PM +0530, Ramalingam C wrote: > > > > > > On 2019-04-05 at 14:32:00 +0200, Greg Kroah-Hartman wrote: > > > > > > > On Fri, Apr 05, 2019 at 04:06:22PM +0530, Ramalingam C wrote: > > > > > > > > On 2019-04-05 at 11:23:00 +0200, Greg Kroah-Hartman wrote: > > > > > > > > > On Fri, Apr 05, 2019 at 02:12:54PM +0530, Ramalingam C wrote: > > > > > > > > > > Functions to create and remove the binary sysfs for class are added. > > > > > > > > > > > > > > > > > > > > These are getting introduced as DRM wants to create the common binary > > > > > > > > > > sysfs across the drm subsystem to handle hdcp srm. > > > > > > > > > > > > > > > > > > Why do you need individual files? That's almost always a sign that you > > > > > > > > > are going to race with userspace in a bad way. Why not just use an > > > > > > > > > attribute group which provides automatic support for this? > > > > > > > > Greg, > > > > > > > > > > > > > > > > Reason behind this move is to have a common srm entry path across all drm > > > > > > > > drivers. And the data fed into this is binary blob. So I am creating a > > > > > > > > binary sysfs "hdcp_srm" at /sys/class/drm/ > > > > > > > > > > > > > > Ah, you want to have a file in your class directory, not your class > > > > > > > device directory. > > > > > > > > > > > > > > No, please do not do that. There's a reason I got rid of those same > > > > > > > types of apis in the past. > > > > > > > > > > > > > > And "binary blobs" are horrid anyway, they are only to be used as a > > > > > > > pass-through to the device itself, from the kernel, no touching the data > > > > > > > at all. If you really need/want this, then put it in the device's > > > > > > > directory as that is where the data is going to, not the kernel "class" > > > > > > > code as it sure as heck better not be doing anything with it. > > > > > > Greg, > > > > > > > > > > > > But here the parsing of the received binary blob is done outside the drm > > > > > > device/cards. This will be generic code for all drm cardx(drivers). And > > > > > > this will provide the service helper functions to the drm drivers for HDCP SRM checking. > > > > > > > > > > Again, the kernel is NOT to be parsing any binary data that comes > > > > > through a sysfs file. If you need such a crazy thing, do it through > > > > > your normal drm ioctl. > > > > > > > > > > > So we prefer to have the binary sysfs at /sys/class/drm/ than inside the > > > > > > cardx folders, so that it will be generic. Could you please suggest a way to achieve that? > > > > > > > > > > What is a "cardx" driver? > > > > Meant card0 card1 etc.. Drm drivers. > > > > > Why can you not do it in a device-specific > > > > > location? Are suddenly _ALL_ DRM drivers going to need this > > > > > information? What is the use case? Who is going to be providing this > > > > > blob and where is it going? What in the kernel uses it? What on the > > > > > hardware uses it? What is it actually? > > > > > > > > This is for HDCP usecase. List of compromised receivers' ID called system > > > > renewability Message (SRM) will be available at userspace which needs to > > > > be passed to be kernel. And this data can be parsed at kernel used > > > > across all DRM drivers to implement the HDCP authentication. Hence we > > > > want generic code to parse the SRM data and provide a helper function to > > > > all DRM drivers to validate their HDCP sink's ID. > > > > > > > > To achieve this we want to keep the sysfs and parsing logic outside the > > > > drm drivers at the class level. > > > > > > > > At present I915 will be using these implementation. in future other DRM > > > > drivers can use the same. > > > > > > Again, binary sysfs files are not for any data that the kernel has to > > > parse at all. So do not use sysfs for this. > > > > So what's the recommend thing then? > > - requrest_firmware blob could work, aside that the blob might change > > (the point of this to allow updates). For some hw you can even stuff > > the SRM into some eprom iirc, so not entirely misfit. Plus kernel > > parses it, so not sure request_firmware is really the right thing. > > I think it is, as this really looks like "firmware" to me as you are > wanting to do something with the buffer and pass it on. > > > - configfs? I never used that one, but the transactional config stuff > > this provides feels like silly amounts of overkill > > Maybe, but why do you want to stuf binary data in your kernel in the > first place? It is configuration, right, why not have it in a format > that you can easily handle it in? Or is that the binary format you > have? Hm, maybe that's the disconnect. It's a blob because that's what the relevant standards body defined it as such. > > - procfs :-) > > Hah, no. > > > - ioctl it definitely isn't, there's no need to open a drm device node > > first and require a special binary to upload the srm if cat > > > $kernfs/srm is all that's needed > > But you want this associtated with a specific drm device, right? So > what's wrong with this? ioctl are generally for the unpriviledged stuff in drm, where multiple client share (mediated by the driver) the single device. So per-client configuration data and stuff like that. Anything that's device-wide (clocks, hw config settings, other stuff like that) we generally stuff into sysfs. Plus some dedicated low-level dev nodes (i2c, dp aux). Also, the "ioctl to configure the device globally" is how dri1 worked, and that stuff is serious nightmare fuel, so we try really hard to avoid that design pattern because history :-) > > - drmfs ... that's some serious amounts of overkill, so nope. > > 300 lines of kernel code for a fs isn't that much :) That's not the problem, it's the "you need to mount this and then cat the file into it and then maybe unmount it again" which doesn't make for the most awesome uapi. > > - per-device sysfs binary blob, silly because it's not per-device, but > > wouldn't need your ack since we could sneak this one in :-) For > > reasonable semantics we could just alias them all to the same storage, > > so that userspace can do a cat > /sys/class/drm/card0/srm instead of > > /sys/class/drm/srm for same effects (for multi gpu systems). > > Again, no, you do not use binary sysfs files for data you want to touch. > Yes, you can try to get away with it, but please do not. See above, that's the canonical format of this things. Of course we could split it up, and let the kernel reassemble it all again (for the case where we need to push it unchanged into hw), but that's somewhat silly. Plus iirc it's also signed, which is somewhat ... hard to fake :-) Anyway I guess request_firmware it is then. We might end up doing a new request_firmware for each modeset, but without the legacy compat cruft (aka usermode helper) request_firmware should be plenty fast enough to not matter I think. And also feels like reasonable uapi. -Daniel
diff --git a/drivers/base/class.c b/drivers/base/class.c index d8a6a5864c2e..b0f37de16a14 100644 --- a/drivers/base/class.c +++ b/drivers/base/class.c @@ -83,6 +83,23 @@ static struct kobj_type class_ktype = { /* Hotplug events for classes go to the class subsys */ static struct kset *class_kset; +int class_create_bin_file(struct class *cls, const struct bin_attribute *attr) +{ + int error; + + if (cls) + error = sysfs_create_bin_file(&cls->p->subsys.kobj, attr); + else + error = -EINVAL; + return error; +} + +void class_remove_bin_file(struct class *cls, const struct bin_attribute *attr) +{ + if (cls) + sysfs_remove_bin_file(&cls->p->subsys.kobj, attr); +} + int class_create_file_ns(struct class *cls, const struct class_attribute *attr, const void *ns) @@ -577,6 +594,8 @@ int __init classes_init(void) return 0; } +EXPORT_SYMBOL_GPL(class_create_bin_file); +EXPORT_SYMBOL_GPL(class_remove_bin_file); EXPORT_SYMBOL_GPL(class_create_file_ns); EXPORT_SYMBOL_GPL(class_remove_file_ns); EXPORT_SYMBOL_GPL(class_unregister); diff --git a/include/linux/device.h b/include/linux/device.h index b6c6a4634801..004c8064a78a 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -485,6 +485,10 @@ struct class_attribute { #define CLASS_ATTR_WO(_name) \ struct class_attribute class_attr_##_name = __ATTR_WO(_name) +extern int __must_check class_create_bin_file(struct class *class, + const struct bin_attribute *attr); +extern void class_remove_bin_file(struct class *class, + const struct bin_attribute *attr); extern int __must_check class_create_file_ns(struct class *class, const struct class_attribute *attr, const void *ns);
Functions to create and remove the binary sysfs for class are added. These are getting introduced as DRM wants to create the common binary sysfs across the drm subsystem to handle hdcp srm. Signed-off-by: Ramalingam C <ramalingam.c@intel.com> cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/base/class.c | 19 +++++++++++++++++++ include/linux/device.h | 4 ++++ 2 files changed, 23 insertions(+)