<!DOCTYPE html>
<html><head>
<meta charset="UTF-8">
</head><body><p>Hello again,</p><p>on Fri, 13 Apr 2018 18:35:58, Nico Huber <nico.h@gmx.de> wrote:<br>> I think our primary focus should be to define a common interface, first,<br>> for different implementations. There's already coreboot's "option table"<br>> that is stored in RTC/CMOS RAM. Marcello has started some work to use<br>> VPD as a backend [1]. There's your implementation; and finally, Felix<br>> and me discussed a storage format specifically for dependable updates on<br>> NOR flashes [2].</p><p>Thanks for the information, those patches are useful. I agree we should<br>define a more flexible interface. There is even FIXME about it in <option.h><br>header. :) I've seen a solution from VPD based implementation ([1]) and<br>adapted ours to implement similar interface. Generally, I think it doesn't<br>need to change much, however:</p><p>1. get_option() function gets destination pointer (void*), but no maximum size,<br>I think we should add size argument to avoid buffer overflow. I assume that<br>we want to allow reading more than one byte. We could also consider adding<br>functions that would also expect specific type of value (boolean, integer) and<br>return suitable type.</p><p>2. In [1], I see there is get_option() made as a wrapper to get_option_cmos()<br>function. Personaly I'd do it so:</p><p>in option.h:</p><p>#if CONFIG_USE_OPTION_TABLE<br> <br>enum cb_err get_option(void *dest, size_t max_size, const char *key);<br> <br>#else<br> <br>inline enum cb_err get_option(void *dest, size_t max_size, const char *key) {<br>return CB_OTABLE_DISABLED;<br>}<br> <br>#endif</p><p>I've changed get_option() prototype. I removed CMOS from the return value, because<br>I think we should keep them implementation independent. With such a header we<br>could link module of choice with proper implementation depending on constant<br>like CONFIG_OPTION_TABLE_BACKEND. It would simplify headers and let us avoid<br>many substitutions. It would also simplify testing if the feature is on. Testing<br>many options at the same time would be inconvenient.</p><p>On the other hand, having backend functions with different names could make code<br>searching a little bit easier. And perhaps I miss something important too?</p><p><br>> Personally, I don't care about the space needed. I would wait with the<br>> implementation until somebody really needs it packed tight.</p><p>I didn't make myself clear. I wrote that because, somewhere in the discussion,<br>fragmentation issue was raised. Keeping small file with static schema would help<br>avoid that problem.</p><p><br>on Fri, 13 Apr 2018 21:55:15, jwerner@google.com wrote:<br>> This is a very important part to keep in mind when we are talking UART.<br>> There has been an attempt inside the Chrome OS team before to make the UART<br>> runtime-configurable, and it fell flat on its face because of this.</p><p>Perhaps we should provide in Kconfig that CBFS implementation is possible.</p><p>Best regards<br>Bartek Pastudzki<br>3mdeb</p></body></html>