| Message ID | 8bc233b19d08308c01015936841d9be8d4441ece.1426973658.git.richardcochran@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 03/21/2015 05:39 PM, Richard Cochran wrote: > This driver calls code (via gxio_mpipe_get/set_timestamp) that makes > the assumption that the tv_sec field is 64 bits wide. So apparently > this driver is 64 bit only. So maybe this driver and device are ready > for 2038, but maybe not. > > Not even compile tested. > > Signed-off-by: Richard Cochran<richardcochran@gmail.com> This driver is 64-bit only. Would it make more sense to just change the accessors from gettime/settime to gettime/settime64 and nothing else, i.e. rely on the current behavior that timespec and timespec64 are the same type, and trust the compiler to fail it if somehow someone tried to build this driver into a 32-bit kernel? In any case, if you think it's better as-is, you can certainly have my Acked-by: Chris Metcalf <cmetcalf@ezchip.com>
On Mon, Mar 23, 2015 at 12:02:03PM -0400, Chris Metcalf wrote: > This driver is 64-bit only. Would it make more sense to just change > the accessors from gettime/settime to gettime/settime64 and nothing > else, i.e. rely on the current behavior that timespec and timespec64 > are the same type, and trust the compiler to fail it if somehow someone > tried to build this driver into a 32-bit kernel? On the one hand, I think the best way would be for gxio_mpipe_get/set_timestamp to also take a timespec64, because it makes the width clear. With a plain old timespec, you have to remember whether you are 64 bit or not. However, but I couldn't tell if changing gxio_mpipe_get/set_timestamp would work for other callers or not. If the driver is 64 bit only, shouldn't that be reflected in the Kconfig? On the other hand, I wouldn't mind the change you suggest, if it were super obvious that the code is 64 bit only. It was not obvious to me. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/23/2015 12:58 PM, Richard Cochran wrote: > On Mon, Mar 23, 2015 at 12:02:03PM -0400, Chris Metcalf wrote: >> >This driver is 64-bit only. Would it make more sense to just change >> >the accessors from gettime/settime to gettime/settime64 and nothing >> >else, i.e. rely on the current behavior that timespec and timespec64 >> >are the same type, and trust the compiler to fail it if somehow someone >> >tried to build this driver into a 32-bit kernel? > On the one hand, I think the best way would be for > gxio_mpipe_get/set_timestamp to also take a timespec64, because it > makes the width clear. With a plain old timespec, you have to > remember whether you are 64 bit or not. However, but I couldn't tell > if changing gxio_mpipe_get/set_timestamp would work for other callers > or not. If you're suggesting changing the gxio_mpipe_xxx_timespec routines to take a timespec64, that sounds reasonable to me, particularly in conjunction with your new gettime64 etc API. As far as I know, the tilegx driver is the only client of those gxio routines. > If the driver is 64 bit only, shouldn't that be reflected in the > Kconfig? Well, the tilegx driver is only supported for the tilegx architecture, which is only 64-bit for kernel-space. It may be a bit confusing because the actual driver is tile_net, which has separate implementations for tilegx (64-bit) and tilepro (32-bit). It's not immediately clear how to make that more obvious. > On the other hand, I wouldn't mind the change you suggest, if it were > super obvious that the code is 64 bit only. It was not obvious to me. I guess that brings us back to changing the gxio_xxx APIs.
On Mon, Mar 23, 2015 at 01:26:22PM -0400, Chris Metcalf wrote: > As far as I know, the tilegx driver is the only client of those gxio > routines. ... > I guess that brings us back to changing the gxio_xxx APIs. Ok, I'll do that for the next round. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/tile/tilegx.c b/drivers/net/ethernet/tile/tilegx.c index bea8cd2..614ea83 100644 --- a/drivers/net/ethernet/tile/tilegx.c +++ b/drivers/net/ethernet/tile/tilegx.c @@ -838,24 +838,28 @@ static int ptp_mpipe_adjtime(struct ptp_clock_info *ptp, s64 delta) return ret; } -static int ptp_mpipe_gettime(struct ptp_clock_info *ptp, struct timespec *ts) +static int ptp_mpipe_gettime(struct ptp_clock_info *ptp, + struct timespec64 *ts64) { int ret = 0; struct mpipe_data *md = container_of(ptp, struct mpipe_data, caps); + struct timespec ts; mutex_lock(&md->ptp_lock); - if (gxio_mpipe_get_timestamp(&md->context, ts)) + if (gxio_mpipe_get_timestamp(&md->context, &ts)) ret = -EBUSY; mutex_unlock(&md->ptp_lock); + *ts64 = timespec_to_timespec64(ts); return ret; } static int ptp_mpipe_settime(struct ptp_clock_info *ptp, - const struct timespec *ts) + const struct timespec64 *ts64) { int ret = 0; + struct timespec ts = timespec64_to_timespec(*ts64); struct mpipe_data *md = container_of(ptp, struct mpipe_data, caps); mutex_lock(&md->ptp_lock); - if (gxio_mpipe_set_timestamp(&md->context, ts)) + if (gxio_mpipe_set_timestamp(&md->context, &ts)) ret = -EBUSY; mutex_unlock(&md->ptp_lock); return ret; @@ -876,8 +880,8 @@ static struct ptp_clock_info ptp_mpipe_caps = { .pps = 0, .adjfreq = ptp_mpipe_adjfreq, .adjtime = ptp_mpipe_adjtime, - .gettime = ptp_mpipe_gettime, - .settime = ptp_mpipe_settime, + .gettime64 = ptp_mpipe_gettime, + .settime64 = ptp_mpipe_settime, .enable = ptp_mpipe_enable, };
This driver calls code (via gxio_mpipe_get/set_timestamp) that makes the assumption that the tv_sec field is 64 bits wide. So apparently this driver is 64 bit only. So maybe this driver and device are ready for 2038, but maybe not. Not even compile tested. Signed-off-by: Richard Cochran <richardcochran@gmail.com> --- drivers/net/ethernet/tile/tilegx.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)