From: "Luis R. Rodriguez" mcgrof@do-not-panic.com
I maintain the the compat-drivers project [0] which aims at backporting the Linux kernel drivers down to older kernels, automatically [1]. Thanks to Ozan Caglayan as a GSoC project we now backport DRM drivers.
The initial framework we had set up to help with the automatic juice was simply quilt refresh to help with hunk offsets, later it consisted of writing cleaner diffs, then compartamentalizing shared differences into a compat backport module [2].
Recently I looked into Coccinelle SmPL to help push for collateral evolutions on the Linux kernel to be written when possible with SmPL [3] given that, as discussed with Julia, the very inverse of the grammar may allow us to backport that collateral evolution on older kernels. I'm still experimenting with that, but another benefit of studying INRIA's Coccinelle work is that the concept of collateral evolutions is now formalized and as I've studied their work I'm realizing we have different categories for collateral evolutions. From what I've experienced through the backport work, data structure changes are the more difficult collateral evolutions to backport. Instead of updates on our compat module these require manual diffs... One strategy to simplify backporting these data structure changes however was to replace the uses of the new elements on the data structure with static inlines and requiring the heavy changes to be implementd on a helper. That is, we actually simplfied the backport by changing the form of the collateral evolution.
The new commit by Jesse that extended the fb_info with a skip_vt_switch element is the simplest example of a data structure expansion. We'd backport this by adding a static inline to compat so that new kernels muck with the new element and for older kernels this would be a no-op. This reduces the size of the backport and unclutters the required patch with #idefs, and insteads leaves only a replacement of the usage of the new elements with a static inline, this however would still be required on our end:
- info->skip_vt_switch = true; + fb_enable_skip_vt_switch(info);
So we'd then have to just add this static inline change for each new driver... There may be a way to get SmPL to do this for us...
However... if the static inline makes it upstream it means 0 changes are required for *any* driver!
I've been reluctant to request a change upstream because of these side backport benefits but due to this case's simplcity I figured I'd shoot this out for review now.
A more elaborate example would be the netdev_attach_ops() which is not yet upstream. This exands to this simple inline for new kernels:
static inline void netdev_attach_ops(struct net_device *dev, const struct net_device_ops *ops) { dev->netdev_ops = ops; }
For older kernels this expands to:
void netdev_attach_ops(struct net_device *dev, const struct net_device_ops *ops) { dev->open = ops->ndo_open; dev->init = ops->ndo_init; dev->stop = ops->ndo_stop; dev->hard_start_xmit = ops->ndo_start_xmit; dev->change_rx_flags = ops->ndo_change_rx_flags; dev->set_multicast_list = ops->ndo_set_multicast_list; dev->validate_addr = ops->ndo_validate_addr; dev->do_ioctl = ops->ndo_do_ioctl; dev->set_config = ops->ndo_set_config; dev->change_mtu = ops->ndo_change_mtu; dev->set_mac_address = ops->ndo_set_mac_address; dev->tx_timeout = ops->ndo_tx_timeout; if (ops->ndo_get_stats) dev->get_stats = ops->ndo_get_stats; dev->vlan_rx_register = ops->ndo_vlan_rx_register; dev->vlan_rx_add_vid = ops->ndo_vlan_rx_add_vid; dev->vlan_rx_kill_vid = ops->ndo_vlan_rx_kill_vid; #ifdef CONFIG_NET_POLL_CONTROLLER dev->poll_controller = ops->ndo_poll_controller; #endif
#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,27)) dev->select_queue = ops->ndo_select_queue; #endif }
Even though we have the static inline in compat it still means every driver must be changed to use it. For example:
--- a/drivers/net/usb/rndis_host.c +++ b/drivers/net/usb/rndis_host.c @@ -358,7 +358,7 @@ generic_rndis_bind(struct usbnet *dev, s dev->rx_urb_size &= ~(dev->maxpacket - 1); u.init->max_transfer_size = cpu_to_le32(dev->rx_urb_size);
- net->netdev_ops = &rndis_netdev_ops; + netdev_attach_ops(net, &rndis_netdev_ops);
retval = rndis_command(dev, u.header, CONTROL_BUFFER_SIZE); if (unlikely(retval < 0)) {
This is just one driver. The patch that deals with this collateral evolution is now 290 lines. I've been working with Julia to express these type of tranformations as SmPL grammar, seeing if its possible to use the inverse of SmPL in the long run, and so on... but in the end, if we add a static inline upstream for small changes like these -- we'd end up requring zero changes for the backport of these type of collateral evolutions.
This should mean less bugs and cleaner backport code and maintenance.
Curious if video guys would care to accept this as an example simple test case to help with the project with this respective small collateral evolution and test case.
What follows are patches that deal with the changes on all the projects. I'll apply the compat and compat-driver patches now as these are required, but if my fb patch (patch #4 in this series) is accepted it'd simpify backporting this collateral evolution for all video drivers tremendously.
[0] https://backports.wiki.kernel.org [1] http://www.do-not-panic.com/2012/08/automatically-backporting-linux-kernel.h... [2] https://git.kernel.org/cgit/linux/kernel/git/mcgrof/compat.git/ [3] http://www.do-not-panic.com/2012/08/optimizing-backporting-collateral.html
linux-next:
Luis R. Rodriguez (1): fb: add helpers to enable and test for the skip_vt_switch
drivers/gpu/drm/i915/intel_fb.c | 2 +- drivers/video/fbmem.c | 2 +- include/linux/fb.h | 10 ++++++++++ 3 files changed, 12 insertions(+), 2 deletions(-)
compat:
Luis R. Rodriguez (1): compat: backport fb_info->skip_vt_switch using a static inline
include/linux/compat-3.10.h | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
compat-drivers:
Luis R. Rodriguez (2): compat-drivers: backport fb_info->skip_vt_switch using ifdefs compat-drivers: simplify backport fb_info->skip_vt_switch CE
.../drm/0001-fb-info-vt_switch.patch | 56 ++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 patches/collateral-evolutions/drm/0001-fb-info-vt_switch.patch
From: "Luis R. Rodriguez" mcgrof@do-not-panic.com
Commit 3cf2667 as of next-20130301 extended the struct fb_info with a skip_vt_switch to allow drivers to skip the VT switch at suspend/resume time. For older kernels we can skip this as all this switch does is call pm_vt_switch_required() with true or false depending on this new flag and later pm_vt_switch_unregister() would not have been made.
This patch cannot be broken down further so I'm pegging this as the first one with 4 digits under the DRM folder for collateral evolutions. This reflects its as atomic as is possible. As we'll see on the next commit, these type of collateral evolutions can best be backported not by keeping ifdef's as below but instead by using a wrapper caller, to help reduce with the amount of lines of code we need. If a static inline is added upstream for these changes, then no code is required for backporting, at all, we'd just implement the static inline later upstream as a no-op.
The tradeoffs to consider for this is if we can live with these practices upstream, we may be able to support full subsystems only with a compat module, and no need for patches. This also means less code and likely less bugs on the distribution front when backporting is required. At least IMHO this may be worthy to consider at least to support kernels listed as supported on kernel.org. We could just leave the ifdef hell to older unsupported kernels.
Relevant commits below, starting with the first one that added this new collateral evolution.
commit 3cf2667b9f8b2c2fe298a427deb399e52321da6b Author: Jesse Barnes jbarnes@virtuousgeek.org Date: Mon Feb 4 13:37:21 2013 +0000
fb: add support for drivers not needing VT switch at suspend/resume time
Use the new PM routines to indicate whether we need to VT switch at suspend and resume time. When a new driver is bound, set its flag accordingly, and when unbound, remove it from the PM's console tracking list.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org Acked-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
commit 24576d23976746cb52e7700c4cadbf4bc1bc3472 Author: Jesse Barnes jbarnes@virtuousgeek.org Date: Tue Mar 26 09:25:45 2013 -0700
drm/i915: enable VT switchless resume v3
With the other bits in place, we can do this safely.
v2: disable backlight on suspend to prevent premature enablement on resume v3: disable CRTCs on suspend to allow RTD3 (Kristen)
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org Reviewed-by: Rodrigo Vivi rodrigo.vivi@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Cc: cocci@systeme.lip6.fr Cc: backports@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: Julia Lawall julia.lawall@lip6.fr Cc: Rodrigo Vivi rodrigo.vivi@gmail.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Jesse Barnes jbarnes@virtuousgeek.org Cc: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Luis R. Rodriguez mcgrof@do-not-panic.com --- .../drm/0001-fb-info-vt_switch.patch | 73 ++++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 patches/collateral-evolutions/drm/0001-fb-info-vt_switch.patch
diff --git a/patches/collateral-evolutions/drm/0001-fb-info-vt_switch.patch b/patches/collateral-evolutions/drm/0001-fb-info-vt_switch.patch new file mode 100644 index 0000000..cdced87 --- /dev/null +++ b/patches/collateral-evolutions/drm/0001-fb-info-vt_switch.patch @@ -0,0 +1,73 @@ +Commit 3cf2667 as of next-20130301 extended the struct fb_info +with a skip_vt_switch to allow drivers to skip the VT switch +at suspend/resume time. For older kernels we can skip this +as all this switch does is call pm_vt_switch_required() with true +or false depending on this new flag and later +pm_vt_switch_unregister() would not have been made. + +This patch cannot be broken down further so I'm pegging +this as the first one with 4 digits under the DRM folder +for collateral evolutions. This reflects its as atomic as +is possible. As we'll see on the next commit, these type +of collateral evolutions can best be backported not by +keeping ifdef's as below but instead by using a wrapper +caller, to help reduce with the amount of lines of code +we need. If a static inline is added upstream for these +changes, then no code is required for backporting, at all, +we'd just implement the static inline later upstream as +a no-op. + +The tradeoffs to consider for this is if we can live with +these practices upstream, we may be able to support full +subsystems only with a compat module, and no need for +patches. This also means less code and likely less bugs +on the distribution front when backporting is required. +At least IMHO this may be worthy to consider at least to +support kernels listed as supported on kernel.org. We could +just leave the ifdef hell to older unsupported kernels. + +Relevant commits below, starting with the first one that +added this new collateral evolution. + +commit 3cf2667b9f8b2c2fe298a427deb399e52321da6b +Author: Jesse Barnes jbarnes@virtuousgeek.org +Date: Mon Feb 4 13:37:21 2013 +0000 + + fb: add support for drivers not needing VT switch at suspend/resume time + + Use the new PM routines to indicate whether we need to VT switch at suspend + and resume time. When a new driver is bound, set its flag accordingly, + and when unbound, remove it from the PM's console tracking list. + + Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org + Acked-by: Rafael J. Wysocki rafael.j.wysocki@intel.com + Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch + +commit 24576d23976746cb52e7700c4cadbf4bc1bc3472 +Author: Jesse Barnes jbarnes@virtuousgeek.org +Date: Tue Mar 26 09:25:45 2013 -0700 + + drm/i915: enable VT switchless resume v3 + + With the other bits in place, we can do this safely. + + v2: disable backlight on suspend to prevent premature enablement on resume + v3: disable CRTCs on suspend to allow RTD3 (Kristen) + + Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org + Reviewed-by: Rodrigo Vivi rodrigo.vivi@gmail.com + Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch + +--- a/drivers/gpu/drm/i915/intel_fb.c ++++ b/drivers/gpu/drm/i915/intel_fb.c +@@ -150,8 +150,10 @@ static int intelfb_create(struct drm_fb_ + } + info->screen_size = size; + ++#if (LINUX_VERSION_CODE >= KERNEL_VERSION(3,10,0)) + /* This driver doesn't need a VT switch to restore the mode on resume */ + info->skip_vt_switch = true; ++#endif + + drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth); + drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
From: "Luis R. Rodriguez" mcgrof@do-not-panic.com
Commit 3cf2667 as of next-20130301 extended the struct fb_info with a skip_vt_switch to allow drivers to skip the VT switch at suspend/resume time. For older kernels we can skip this as all this switch does is call pm_vt_switch_required() with true or false depending on this new flag and later pm_vt_switch_unregister() would not have been made.
compat-drivers was backporting this using #ifdef's but by integrating a static inline we'd reduce the number of lines to backport to just 1 line replacement. This would be something like:
- info->skip_vt_switch = true; + fb_enable_skip_vt_switch(info);
For kernels >= 3.10 we'd set the attribute as we do upstream, for older kernels this would be a no-op.
If this static inline would have been added upstream it would have meant this collateral evolution would require just adding a no-op static inline to backport, and no changes as the above example hunk for every driver that requires the change.
If this static inline ends up upstream *now* it means we do *not* require the type of hunk above for every driver that requires the change.
All the code would be left intact !
This is a linux-next 'data structure element collateral evolution'.
Cc: cocci@systeme.lip6.fr Cc: backports@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: Julia Lawall julia.lawall@lip6.fr Cc: Rodrigo Vivi rodrigo.vivi@gmail.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Jesse Barnes jbarnes@virtuousgeek.org Cc: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Luis R. Rodriguez mcgrof@do-not-panic.com --- include/linux/compat-3.10.h | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/include/linux/compat-3.10.h b/include/linux/compat-3.10.h index 69ddc11..9abfc4b 100644 --- a/include/linux/compat-3.10.h +++ b/include/linux/compat-3.10.h @@ -7,6 +7,7 @@
#include <linux/scatterlist.h> #include <linux/mm.h> +#include <linux/fb.h>
#define sg_page_iter_page LINUX_BACKPORT(sg_page_iter_page) /** @@ -29,6 +30,46 @@ static inline dma_addr_t sg_page_iter_dma_address(struct sg_page_iter *piter) return sg_dma_address(piter->sg) + (piter->sg_pgoffset << PAGE_SHIFT); }
+/* + * This is a linux-next data structure element collateral evolution, + * we use a wrapper to avoid #ifdef hell to backport it. This allows + * us to use a simple fb_info_skip_vt_switch() replacement for when + * the new data structure element is used. If coccinelle SmPL grammar + * could be used to express the transformation for us on compat-drivers + * it means we'd need to express it only once. If the structure element + * collateral evolution were to be used *at development* time and we'd + * have a way to express the inverse through SmPL we'd be able to + * backport this collateral evolution automatically for any new driver + * that used it. We'd use coccinelle to look for it and do the + * transformations for us based on the original commit (maybe SmPL + * would be listed on the commit log. + * + * We may need the LINUX_BACKPORT() call that adds the backport_ + * prefix for older kernels than 3.10 if distros decide to + * add this same static inline themselves (although unlikely). + */ +#define fb_enable_skip_vt_switch LINUX_BACKPORT(fb_enable_skip_vt_switch) +static inline void fb_enable_skip_vt_switch(struct fb_info *info) +{ +} + +#else /* kernel is >= 3.10 */ +/* + * We'd delete this upstream ever got this, we use our + * backport_ prefix with LINUX_BACKPORT() so that if this + * does get upstream we would not have to add another ifdef + * here for the kernels in between v3.10.. up to the point + * the routine would have gotten added, we'd just delete this + * #else condition completely. If we didn't have this and + * say 3.12 added the static inline upstream, we'd have a + * clash on the backport for 3.12 as the routine would + * already be defined *but* we'd need it for 3.11. + */ +#define fb_enable_skip_vt_switch LINUX_BACKPORT(fb_enable_skip_vt_switch) +static inline void fb_enable_skip_vt_switch(struct fb_info *info) +{ + info->skip_vt_switch = true; +} #endif /* (LINUX_VERSION_CODE < KERNEL_VERSION(3,10,0)) */
#endif /* LINUX_3_10_COMPAT_H */
From: "Luis R. Rodriguez" mcgrof@do-not-panic.com
The collateral evolution (CE) on the fb_info data structure that added the skip_vt_switch element can be simplified further by replacing the #ifdef hell with a static inline.
Furthermore, if the static inline is added upstream it'd mean we can get rid of all these static inline replacements for this data structure element CE.
Cc: cocci@systeme.lip6.fr Cc: backports@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: Julia Lawall julia.lawall@lip6.fr Cc: Rodrigo Vivi rodrigo.vivi@gmail.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Jesse Barnes jbarnes@virtuousgeek.org Cc: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Luis R. Rodriguez mcgrof@do-not-panic.com --- .../drm/0001-fb-info-vt_switch.patch | 25 ++++---------------- 1 file changed, 4 insertions(+), 21 deletions(-)
diff --git a/patches/collateral-evolutions/drm/0001-fb-info-vt_switch.patch b/patches/collateral-evolutions/drm/0001-fb-info-vt_switch.patch index cdced87..39b13d1 100644 --- a/patches/collateral-evolutions/drm/0001-fb-info-vt_switch.patch +++ b/patches/collateral-evolutions/drm/0001-fb-info-vt_switch.patch @@ -8,23 +8,7 @@ pm_vt_switch_unregister() would not have been made. This patch cannot be broken down further so I'm pegging this as the first one with 4 digits under the DRM folder for collateral evolutions. This reflects its as atomic as -is possible. As we'll see on the next commit, these type -of collateral evolutions can best be backported not by -keeping ifdef's as below but instead by using a wrapper -caller, to help reduce with the amount of lines of code -we need. If a static inline is added upstream for these -changes, then no code is required for backporting, at all, -we'd just implement the static inline later upstream as -a no-op. - -The tradeoffs to consider for this is if we can live with -these practices upstream, we may be able to support full -subsystems only with a compat module, and no need for -patches. This also means less code and likely less bugs -on the distribution front when backporting is required. -At least IMHO this may be worthy to consider at least to -support kernels listed as supported on kernel.org. We could -just leave the ifdef hell to older unsupported kernels. +is possible.
Relevant commits below, starting with the first one that added this new collateral evolution. @@ -60,14 +44,13 @@ Date: Tue Mar 26 09:25:45 2013 -0700
--- a/drivers/gpu/drm/i915/intel_fb.c +++ b/drivers/gpu/drm/i915/intel_fb.c -@@ -150,8 +150,10 @@ static int intelfb_create(struct drm_fb_ +@@ -150,8 +150,8 @@ static int intelfb_create(struct drm_fb_ } info->screen_size = size;
-+#if (LINUX_VERSION_CODE >= KERNEL_VERSION(3,10,0)) /* This driver doesn't need a VT switch to restore the mode on resume */ - info->skip_vt_switch = true; -+#endif +- info->skip_vt_switch = true; ++ fb_enable_skip_vt_switch(info);
drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth); drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
From: "Luis R. Rodriguez" mcgrof@do-not-panic.com
This adds helpers to enable and test for the skip_vt_switch. This gets us two things:
1) It allows us to require less collateral evolutions should we need changes on the fb_info data structure later (perhaps a bitmap flag).
2) Allows this feature to be backported with 0 delta required on the upstream drivers, we'd just require the static inline to do a no-op.
1) may be worth while considering, 2) is a new consideration I'm trying to get others to evaluate to help with automatically backporting the Linux kernel. This is the first time I am aware someone argues for it.
Cc: cocci@systeme.lip6.fr Cc: backports@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: Julia Lawall julia.lawall@lip6.fr Cc: Rodrigo Vivi rodrigo.vivi@gmail.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Jesse Barnes jbarnes@virtuousgeek.org Cc: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Luis R. Rodriguez mcgrof@do-not-panic.com --- drivers/gpu/drm/i915/intel_fb.c | 2 +- drivers/video/fbmem.c | 2 +- include/linux/fb.h | 10 ++++++++++ 3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c index 8d81c929..c0f306c 100644 --- a/drivers/gpu/drm/i915/intel_fb.c +++ b/drivers/gpu/drm/i915/intel_fb.c @@ -151,7 +151,7 @@ static int intelfb_create(struct drm_fb_helper *helper, info->screen_size = size;
/* This driver doesn't need a VT switch to restore the mode on resume */ - info->skip_vt_switch = true; + fb_enable_skip_vt_switch(info);
drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth); drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height); diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c index ccd44b0..daffadc 100644 --- a/drivers/video/fbmem.c +++ b/drivers/video/fbmem.c @@ -1645,7 +1645,7 @@ static int do_register_framebuffer(struct fb_info *fb_info) if (!fb_info->modelist.prev || !fb_info->modelist.next) INIT_LIST_HEAD(&fb_info->modelist);
- if (fb_info->skip_vt_switch) + if (fb_skip_vt_switch_isset(fb_info)) pm_vt_switch_required(fb_info->dev, false); else pm_vt_switch_required(fb_info->dev, true); diff --git a/include/linux/fb.h b/include/linux/fb.h index d49c60f..d534966 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -505,6 +505,16 @@ struct fb_info { bool skip_vt_switch; /* no VT switch on suspend/resume required */ };
+static inline void fb_enable_skip_vt_switch(struct fb_info *info) +{ + info->skip_vt_switch = true; +} + +static inline bool fb_skip_vt_switch_isset(struct fb_info *info) +{ + return info->skip_vt_switch; +} + static inline struct apertures_struct *alloc_apertures(unsigned int max_num) { struct apertures_struct *a = kzalloc(sizeof(struct apertures_struct) + max_num * sizeof(struct aperture), GFP_KERNEL);
On Thu, 28 Mar 2013 05:04:26 -0700 "Luis R. Rodriguez" mcgrof@do-not-panic.com wrote:
The new commit by Jesse that extended the fb_info with a skip_vt_switch element is the simplest example of a data structure expansion. We'd backport this by adding a static inline to compat so that new kernels muck with the new element and for older kernels this would be a no-op. This reduces the size of the backport and unclutters the required patch with #idefs, and insteads leaves only a replacement of the usage of the new elements with a static inline, this however would still be required on our end:
info->skip_vt_switch = true;
fb_enable_skip_vt_switch(info);So we'd then have to just add this static inline change for each new driver... There may be a way to get SmPL to do this for us...
Yeah I'm not attached to the direct structure reference; a couple of inlines are just as easy to read. So no argument from me.
Thanks,
On Thu, 28 Mar 2013, Jesse Barnes wrote:
On Thu, 28 Mar 2013 05:04:26 -0700 "Luis R. Rodriguez" mcgrof@do-not-panic.com wrote:
The new commit by Jesse that extended the fb_info with a skip_vt_switch element is the simplest example of a data structure expansion. We'd backport this by adding a static inline to compat so that new kernels muck with the new element and for older kernels this would be a no-op. This reduces the size of the backport and unclutters the required patch with #idefs, and insteads leaves only a replacement of the usage of the new elements with a static inline, this however would still be required on our end:
info->skip_vt_switch = true;
fb_enable_skip_vt_switch(info);So we'd then have to just add this static inline change for each new driver... There may be a way to get SmPL to do this for us...
@@ type of info *info; @@
- info->skip_vt_switch = true; + fb_enable_skip_vt_switch(info);
for whatever the type of info is.
Then I guess there would be a similar rule for the false case?
julia
Yeah I'm not attached to the direct structure reference; a couple of inlines are just as easy to read. So no argument from me.
Thanks,
Jesse Barnes, Intel Open Source Technology Center
On Thu, Mar 28, 2013 at 9:19 AM, Julia Lawall julia.lawall@lip6.fr wrote:
On Thu, 28 Mar 2013, Jesse Barnes wrote:
- info->skip_vt_switch = true; + fb_enable_skip_vt_switch(info);So we'd then have to just add this static inline change for each new driver... There may be a way to get SmPL to do this for us...
@@ type of info *info; @@
info->skip_vt_switch = true;
fb_enable_skip_vt_switch(info);for whatever the type of info is.
Thanks Julia! I'll be sure to try to add this to compat-drivers if the upstream fb patch is not accepted. If it is accepted we would not need this at all!
Then I guess there would be a similar rule for the false case?
Nope, see that's the proactive strategy taken by the static inline and hence the patch. compat would have a static inline for both cases, and for the false case it'd be a no-op. If accepted upstream though then we would not need any changes for this collateral evolution. However *spotting* these collateral evolutions and giving you SmPL for them as a proactive strategy might be good given that if these type of patches are indeed welcomed upstream we'd then be able to address these as secondary steps. If they are not accepted then indeed we'd use them to backport that collateral evolution through both compat (adds the static inlines) and compat-drivers (the SmPL).
Luis
On Thu, 28 Mar 2013, Luis R. Rodriguez wrote:
On Thu, Mar 28, 2013 at 9:19 AM, Julia Lawall julia.lawall@lip6.fr wrote:
On Thu, 28 Mar 2013, Jesse Barnes wrote:
- info->skip_vt_switch = true; + fb_enable_skip_vt_switch(info);So we'd then have to just add this static inline change for each new driver... There may be a way to get SmPL to do this for us...
@@ type of info *info; @@
info->skip_vt_switch = true;
fb_enable_skip_vt_switch(info);for whatever the type of info is.
Thanks Julia! I'll be sure to try to add this to compat-drivers if the upstream fb patch is not accepted. If it is accepted we would not need this at all!
Then I guess there would be a similar rule for the false case?
Nope, see that's the proactive strategy taken by the static inline and hence the patch. compat would have a static inline for both cases, and for the false case it'd be a no-op. If accepted upstream though then we would not need any changes for this collateral evolution. However *spotting* these collateral evolutions and giving you SmPL for them as a proactive strategy might be good given that if these type of patches are indeed welcomed upstream we'd then be able to address these as secondary steps. If they are not accepted then indeed we'd use them to backport that collateral evolution through both compat (adds the static inlines) and compat-drivers (the SmPL).
Probably I am missing something, since I haven't looked at the code in detail, bu wouldn't it be nicer to have a function call for the false case, if there is a function call for the true case? In looking at the code, one could wonder why things are not done in a parallel way.
julia
On Thu, Mar 28, 2013 at 11:10 AM, Julia Lawall julia.lawall@lip6.fr wrote:
On Thu, 28 Mar 2013, Luis R. Rodriguez wrote:
Thanks Julia! I'll be sure to try to add this to compat-drivers if the upstream fb patch is not accepted. If it is accepted we would not need this at all!
Then I guess there would be a similar rule for the false case?
Nope, see that's the proactive strategy taken by the static inline and hence the patch. compat would have a static inline for both cases, and for the false case it'd be a no-op. If accepted upstream though then we would not need any changes for this collateral evolution. However *spotting* these collateral evolutions and giving you SmPL for them as a proactive strategy might be good given that if these type of patches are indeed welcomed upstream we'd then be able to address these as secondary steps. If they are not accepted then indeed we'd use them to backport that collateral evolution through both compat (adds the static inlines) and compat-drivers (the SmPL).
Probably I am missing something, since I haven't looked at the code in detail, bu wouldn't it be nicer to have a function call for the false case, if there is a function call for the true case?
Yes, and indeed we have that, its the same function call, in the negative case its a no-op, in the newer kernels it wraps to modifying the element as in the original code.
In looking at the code, one could wonder why things are not done in a parallel way.
Not sure I get this.
Luis
On Thu, 28 Mar 2013, Luis R. Rodriguez wrote:
On Thu, Mar 28, 2013 at 11:10 AM, Julia Lawall julia.lawall@lip6.fr wrote:
On Thu, 28 Mar 2013, Luis R. Rodriguez wrote:
Thanks Julia! I'll be sure to try to add this to compat-drivers if the upstream fb patch is not accepted. If it is accepted we would not need this at all!
Then I guess there would be a similar rule for the false case?
Nope, see that's the proactive strategy taken by the static inline and hence the patch. compat would have a static inline for both cases, and for the false case it'd be a no-op. If accepted upstream though then we would not need any changes for this collateral evolution. However *spotting* these collateral evolutions and giving you SmPL for them as a proactive strategy might be good given that if these type of patches are indeed welcomed upstream we'd then be able to address these as secondary steps. If they are not accepted then indeed we'd use them to backport that collateral evolution through both compat (adds the static inlines) and compat-drivers (the SmPL).
Probably I am missing something, since I haven't looked at the code in detail, bu wouldn't it be nicer to have a function call for the false case, if there is a function call for the true case?
Yes, and indeed we have that, its the same function call, in the negative case its a no-op, in the newer kernels it wraps to modifying the element as in the original code.
In looking at the code, one could wonder why things are not done in a parallel way.
Not sure I get this.
I looked in today's linux-next, and there seems to be only one initialization of this field, to true, and one test of this field. So perhaps the case for setting the field to false just isn't needed.
julia
On Thu, Mar 28, 2013 at 3:29 PM, Julia Lawall julia.lawall@lip6.fr wrote:
On Thu, 28 Mar 2013, Luis R. Rodriguez wrote:
On Thu, Mar 28, 2013 at 11:10 AM, Julia Lawall julia.lawall@lip6.fr wrote:
On Thu, 28 Mar 2013, Luis R. Rodriguez wrote:
Thanks Julia! I'll be sure to try to add this to compat-drivers if the upstream fb patch is not accepted. If it is accepted we would not need this at all!
Then I guess there would be a similar rule for the false case?
Nope, see that's the proactive strategy taken by the static inline and hence the patch. compat would have a static inline for both cases, and for the false case it'd be a no-op. If accepted upstream though then we would not need any changes for this collateral evolution. However *spotting* these collateral evolutions and giving you SmPL for them as a proactive strategy might be good given that if these type of patches are indeed welcomed upstream we'd then be able to address these as secondary steps. If they are not accepted then indeed we'd use them to backport that collateral evolution through both compat (adds the static inlines) and compat-drivers (the SmPL).
Probably I am missing something, since I haven't looked at the code in detail, bu wouldn't it be nicer to have a function call for the false case, if there is a function call for the true case?
Yes, and indeed we have that, its the same function call, in the negative case its a no-op, in the newer kernels it wraps to modifying the element as in the original code.
In looking at the code, one could wonder why things are not done in a parallel way.
Not sure I get this.
I looked in today's linux-next, and there seems to be only one initialization of this field, to true, and one test of this field. So perhaps the case for setting the field to false just isn't needed.
Oh sorry now I get what you mean! I thought about this -- and yes I decided to not add a bool false setting for the structure field given that the assumption is this would not be something dynamic, and data structure would be kzalloc()'d so by default they are 0.
Luis
I looked in today's linux-next, and there seems to be only one initialization of this field, to true, and one test of this field. So perhaps the case for setting the field to false just isn't needed.
Oh sorry now I get what you mean! I thought about this -- and yes I decided to not add a bool false setting for the structure field given that the assumption is this would not be something dynamic, and data structure would be kzalloc()'d so by default they are 0.
What do you do about the test, though?
julia
On Thu, Mar 28, 2013 at 11:21 PM, Julia Lawall julia.lawall@lip6.fr wrote:
I looked in today's linux-next, and there seems to be only one initialization of this field, to true, and one test of this field. So perhaps the case for setting the field to false just isn't needed.
Oh sorry now I get what you mean! I thought about this -- and yes I decided to not add a bool false setting for the structure field given that the assumption is this would not be something dynamic, and data structure would be kzalloc()'d so by default they are 0.
What do you do about the test, though?
Return the value.
Luis
dri-devel@lists.freedesktop.org
-
Jesse Barnes -
Julia Lawall -
Luis R. Rodriguez