Differential D43370
subr_bus: introduce device_set_descf() and modify allocation logic Authored by christos on Jan 8 2024, 10:53 PM. Tags None Referenced Files
Subscribers
Details device_set_descf() is a printf-like version of device_set_desc(). Allocation code has been transferred from device_set_desc_internal() to Sponsored by: The FreeBSD Foundation
Diff Detail
Event Timeline
Comment Actions I like it. But please wait for Warner to chime in. I can help convert device_set_desc_copy() callers to use device_set_descf(). Many of them become a bit simpler. Comment Actions what's a use case for this? I think I know, but I thought I'd ask to make sure.
Comment Actions tl;dr: I like the idea, but there's minor details that I need to quibble over. Too bad the quibbles were wrong. Hmmm... but it also looks like I misremembered the original behavior and misread the code. so ignore my first comments... The other alternative is to always copy the string value. Then we don't need a new function at all. We can make the current one just accept varargs... Though I suppose that since it's tagged as printf we'll get format warnings because those strings come from tables, so maybe we do need a new function after all.... If we really wanted to do this, we could do it with macro magic though, at the cost of more symbols being exposed... jhb? Is it worth it? The name ending in 'f' isn't typical. That's usually for 'flags'. There's one other place that uses 'f' for format. There's no other examples, though, of modern apis that do anything: either they've always taken the extra args, or they are ancient and prepend 'v' like vprintf, which isn't quite the same thing... So the name is fine. adding jhb@ to see if he thinks it would be a cleaner interface to do this, or the varargs trick with the same name. I ticked accept because I think I'm good either way.
Comment Actions Address Warner's comments:
Comment Actions I would support making device_set_desc() just be device_set_descf() that always does a copy. Though you'd probably have to fix some places that do table lookups to use "%s" to format the string from the table if you did that.
Comment Actions So there's approx 1380 calls to device_set_desc today. of the 330 that remain: So that's a lot of work to not do something clever as a transition aide for calls with just 2 arg esp for MFCability... but it's also a lot of work to roll this out to places that can use it to simplify things... So I'm not sure what these numbers tell me. Comment Actions An AMD64 generic kernel and modules generates 242 error files if I change device_set_desc to be varadic with the printflike markings. Comment Actions I think some of the first modules that can initially benefit from device_set_descf() which do not require that much work are the audio drivers. By the looks of it, it might be too much (perhaps unnecessary) work to do this for every single module that could benefit from this function.
Comment Actions So let's try converting some drivers to use this function and see what the diff looks like? The sound drivers seem like a good place to start.
Comment Actions Here. This is without the use of device_set_descf() in uaudio. Comment Actions I think we can certainly defer unifying the APIs given it looks pretty invasive. I think by definition device_set_desc is always safe to use M_WAITOK btw. device_probe is generally run from device_probe_and_attach and device_attach generally speaking can only run in sleepable contexts. Drivers that can add new devices at runtime via hot plug defer adding and removing devices to a taskqueue because there's just too much stuff in attach/detach in general that requires sleeping. device_probe ends up in the same boat, so it really should just be safe to always use M_WAITOK for device_*_desc. That said, the M_WAITOK fixes are also orthogonal. I would at least fix the one explicit M_NOWAIT in the current patch to be M_WAITOK since it's easy to do so now. The asprintf() thing can be fixed either before or after this patch IMO. Comment Actions The one explicit M_NOWAIT in the current patch is carried over from the existing implementation of device_set_desc_copy(). Until all of the callers are audited (which can be done while they're being converted to use device_set_descf()), I would avoid changing the malloc() behaviour. I went through maybe 40 callers just now and agree that it's most likely a safe change, but it's also orthogonal to the patch as proposed. Comment Actions I agree with John more than Mark: Even though it the NOWAIT was copied, it's completely safe to be WAIT. All probe/attach routines have always run in a sleepable context, and will be in the future. Especially for memory allocation. Though the initial probe/attach can't do much if we can't allocate memory (though the system can't do much if we can't allocate enough memory to get through boot). But I won't insist that the new thing not use WAITOK. Seems like we shouldn't be adding cases we need to fix later though. We have way too many of those in CAM, but CAM has a much much more complex history. Comment Actions Actually, the more I think about it, I'll make a followup with the NOWAIT -> WAITOK change... | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||